Conversation
commit: |
|
|
||
| return ( | ||
| <> | ||
| <Component {...props} /> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Also, this withDevTools method is kind of low-key deprecated
There was a problem hiding this comment.
deprecated
in favor of?
There was a problem hiding this comment.
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
| const isEmbedded = useIsEmbeddedRDT() | ||
| if (!hydrated) return <Component /> | ||
|
|
||
| if ((config?.panelMode ?? "auto") === "embedded") { |
There was a problem hiding this comment.
why the default here? you don't really need to check if "auto" === "embedded"
There was a problem hiding this comment.
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. | |||
There was a problem hiding this comment.
how old of a template did you use here? 🤣
There was a problem hiding this comment.
It's just a copy of test-apps/remix-vite 😅
There was a problem hiding this comment.
Oh boy thats embarrassing 😳
| if ((config?.panelMode ?? "auto") === "embedded") { | ||
| return ( | ||
| <RDTEmbeddedContextProvider> | ||
| <Component /> | ||
| </RDTEmbeddedContextProvider> | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| <Component {...props} /> | ||
| {createPortal(<RemixDevTools {...config} />, document.body)} | ||
| </> | ||
| ) | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Primarily because you really want React Context to be granular. One thing I was going to suggest to you was to separate the dispatch context out from the state context.
Having them both in the same context is inefficient, because every useContext() call subscribes the component to rerenders whenever the context value changes. That means that components that only need the dispatch will have to re-render whenever the state changes, even if they never read any state. By breaking those up into separate contexts, you get granular control of what causes a re-render. And since dispatch 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 invoking dispatch!).
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 by RDTContextProvider changes.
There was a problem hiding this comment.
I see, makes sense, okay
2 participants
WIP
Mainly opening this PR for feedback about the approach