Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

expand on embedded mode#155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
rossipedia wants to merge1 commit intoforge-42:main
base:main
Choose a base branch
Loading
fromrossipedia:feat/embedded-mode

Conversation

@rossipedia
Copy link
Contributor

WIP

Mainly opening this PR for feedback about the approach

@pkg-pr-new
Copy link

Open in Stackblitz

yarn add https://pkg.pr.new/forge42dev/Remix-Dev-Tools/remix-development-tools@155.tgz

commit:f42af4b


return(
<>
<Component{...props}/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

not spreading the props over the component actually breaks RR7 support, I learned it the hard way yesterday with line 27 above breaking my rr7 test setup. So props need to be accepted and spread across Component in every return case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Also, this withDevTools method is kind of low-key deprecated

rossipedia reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

deprecated

in favor of?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

WithViteDevTools, this one is undocumented relic used to manually set it up if needed in remix v1, I plan to remove it fully in next major

rossipedia reacted with thumbs up emoji
constisEmbedded=useIsEmbeddedRDT()
if(!hydrated)return<Component/>

if((config?.panelMode??"auto")==="embedded"){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

why the default here? you don't really need to check if "auto" === "embedded"

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Mainly for clarity that the default mode is"auto". It's not technically necessary, happy to remove it if preferred.

@@ -0,0 +1,28 @@
#templates/unstable-vite

⚠️ Remix support for Vite is unstable and not recommended for production.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

how old of a template did you use here? 🤣

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It's just a copy oftest-apps/remix-vite 😅

AlemTuzlak reacted with eyes emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oh boy thats embarrassing 😳

Comment on lines +55 to +69
if((config?.panelMode??"auto")==="embedded"){
return(
<RDTEmbeddedContextProvider>
<Component/>
</RDTEmbeddedContextProvider>
)
}

return(
<>
<Component{...props}/>
{createPortal(<RemixDevTools{...config}/>,document.body)}
</>
)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I've been thinking,RDTContextProvider wraps both the EmbeddedDevTools and the regular ones, why wouldn't you just pop this panelMode option into that and here just not use the createPortal? instead of having 2 contexts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Then you in the vite plugin set it to embedded, that hides this createPortal, and wherever you use you just pass in a prop that the panel is embedded (inside the EmbeddedDevTools to the context) and you get this behavior with less hassle and there is 1 context to read from

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Primarily because you really want React Context to be granular. One thing I was going to suggest to you was to separate thedispatch context out from thestate context.

Having them both in the same context is inefficient, because everyuseContext() call subscribes the component to rerenders whenever the context value changes. That means that components that only need thedispatch will have to re-render whenever thestate changes, even if they never read anystate. By breaking those up into separate contexts, you get granular control of what causes a re-render. And sincedispatch is stable, components that only need to dispatch won't re-render themselves after the state changes (often times in response to those components themselves invokingdispatch!).

With that in mind, having a separate context for "embedded" mode makes sense, since the value of this is essentially static, a component that needs to render differently based on embedded-mode or not won't need to re-render whenever the state managed byRDTContextProvider changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I see, makes sense, okay

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@AlemTuzlakAlemTuzlakAlemTuzlak left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rossipedia@AlemTuzlak

[8]ページ先頭

©2009-2025 Movatter.jp