RRR Step 3 - Hydration #4703
RRR Step 3 - Hydration #4703
Conversation
|
brophdawg11
force-pushed
the
rrr-hydration
branch
from
31beaf4
to
23474db
Compare
brophdawg11
force-pushed
the
feat/rrr-rendering
branch
2 times, most recently
from
3c1a807
to
77e6254
Compare
brophdawg11
force-pushed
the
rrr-hydration
branch
from
23474db
to
ca62930
Compare
brophdawg11
force-pushed
the
feat/rrr-rendering
branch
from
77e6254
to
974bed2
Compare
brophdawg11
force-pushed
the
rrr-hydration
branch
from
7dd22ea
to
3036f2e
Compare
brophdawg11
force-pushed
the
feat/rrr-rendering
branch
from
974bed2
to
fb2d6df
Compare
brophdawg11
force-pushed
the
rrr-hydration
branch
from
baf1dfb
to
9ff7419
Compare
brophdawg11
force-pushed
the
feat/rrr-rendering
branch
from
fb2d6df
to
a340693
Compare
brophdawg11
force-pushed
the
rrr-hydration
branch
from
691c7c9
to
6ed9f3f
Compare
| _isRedirect: true, | ||
| // These were private API for transition manager that are no longer | ||
| // needed with the new router so OK to disappear | ||
| // setCookie: false, | ||
| // type: "loader", |
There was a problem hiding this comment.
Passing integration tests from dev change slightly since we use new private API on location state to detect type
| "useTransition is deprecated in favor of useNavigation as of v1.9.0.", | ||
| name: "@remix-run/react", | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Linter deprecation warning for useTransition - this might be 1.10.0 now though...
There was a problem hiding this comment.
Lets bulk include deprecation warnings in a subsequent release
There was a problem hiding this comment.
3 types of deprecation warnings: eslint, type definitions (jsdoc + ts), and runtime console.warn
There was a problem hiding this comment.
Remove for now, we may add back in for the stable release
| // React knows the order and handles error boundaries normally. | ||
| entryContext.appState.trackBoundaries = false; | ||
| entryContext.appState.trackCatchBoundaries = false; | ||
| router = createBrowserRouter(routes, { hydrationData }); |
There was a problem hiding this comment.
Create our client side router singleton
| }} | ||
| > | ||
| <RouterProvider router={router} fallbackElement={null} /> | ||
| </RemixContext.Provider> |
There was a problem hiding this comment.
Render <RouterProvider> directly
| @@ -764,6 +796,7 @@ import(${JSON.stringify(manifest.entry.module)});`; | |||
| /> | |||
| <script | |||
| {...props} | |||
| suppressHydrationWarning | |||
There was a problem hiding this comment.
Any reason this wasn't there already in dev? We added it while debugging some other integration test hydration failures, which I think came down to the lack of a <head> in our root.tsx but this felt like a good change?
There was a problem hiding this comment.
Leave this in 👍
| data: match.data, | ||
| // Need to grab handle here since we don't have it at client-side route | ||
| // creation time | ||
| handle: routeModules[match.id].handle, |
There was a problem hiding this comment.
Grab handle from the routeModulesCache
| // Have to avoid useMemo here to avoid introducing unstable transition object | ||
| // identities in StrictMode, since navigation will be stable but using | ||
| // [navigation] as the dependency array will _still_ re-run on concurrent | ||
| // renders, and that will create a new object identify for transition | ||
| let lastNavigationRef = React.useRef<Navigation>(); | ||
| let lastTransitionRef = React.useRef<Transition>(); | ||
|
|
||
| if (lastTransitionRef.current && lastNavigationRef.current === navigation) { | ||
| return lastTransitionRef.current; | ||
| } | ||
|
|
||
| lastNavigationRef.current = navigation; | ||
| lastTransitionRef.current = convertNavigationToTransition(navigation); | ||
|
|
||
| return lastTransitionRef.current; |
There was a problem hiding this comment.
Concurrent mode prevents useMemo here since it re-runs even if the navigation doesn't change and creates a new transition object identity 😖
There was a problem hiding this comment.
Lets convert this back to useMemo and update our integration test - but Matt/Jacob to put together a smaller reproduction to discuss separately.
| return lastTransitionRef.current; | ||
| } | ||
|
|
||
| function convertNavigationToTransition(navigation: Navigation): Transition { |
There was a problem hiding this comment.
Mega if/else to convert navigations to transitions
| ["POST", "PUT", "PATCH", "DELETE"].includes(formMethod.toUpperCase()); | ||
|
|
||
| if (state === "idle") { | ||
| if (fetcherRR[" _hasFetcherDoneAnything "] === true) { |
There was a problem hiding this comment.
Needed something here to differentiate fetcher idle/init and idle/done
There was a problem hiding this comment.
For the useFetcher use-case we could do this by tracking some local state (let didAnything = useState(false)).
However, in useFetchers we don't have a stable key to track fetchers so we cant do the same there.
In user application code, they can also derive this "done" state via their own useState if needed.
| // we're considering adding useFetcher({ key }) anyway | ||
| // - Expose a hidden field with a stable identifier on the fetcher | ||
| // like we did for _hasFetcherDoneAnything | ||
| key: "todo-not-implemented-yet", |
There was a problem hiding this comment.
Router doesn't have submission keys - need to think about the best way to tackle this.
Maybe we just add a navigation key to router?
navigation = {
state,
location, // has a key
form*,
key, // populate on _all_ navigations, submisison or not
// does not change on redirects during a naviagtion
}There was a problem hiding this comment.
Might be able to solve this with location.state._submissionKey which would allow us to track this key across submission redirects
There was a problem hiding this comment.
Undocumented API we can leave this out, this can be done with a hidden field in formData to track a submission. Set to key: "" for typing reasons
| }; | ||
| return fetcher; | ||
| } else { | ||
| invariant(false, "nope"); |
There was a problem hiding this comment.
Note to self - make this more descriptive
| // The new router fixes a bug in useTransition where the submission | ||
| // "action" represents the request URL not the state of the <form> in | ||
| // the DOM. Back-port it here to maintain behavior, but useNavigation | ||
| // will fix this bug. | ||
| let url = new URL(formAction, window.location.origin); | ||
|
|
||
| // This typing override should be safe since this is only running for | ||
| // GET submissions and over in @remix-run/router we have an invariant | ||
| // if you have any non-string values in your FormData when we attempt | ||
| // to convert them to URLSearchParams | ||
| url.search = new URLSearchParams( | ||
| formData.entries() as unknown as [string, string][] | ||
| ).toString(); |
There was a problem hiding this comment.
This bug is fixed in useNavigation but we wanted to keep the bug behavior in useTransition
| // TODO: There's a bug in @remix-run/router here at the moment where the | ||
| // loader Request keeps method POST after a submission. Matt has a local | ||
| // fix but this does the trick for now. Once the fix is merged to the | ||
| // router, we can remove the isAction param and use the method here | ||
| // if (request.method !== "GET") { |
There was a problem hiding this comment.
This is no longer the case - Matt to update
|
|
||
| // TODO: Dropped credentials:"same-origin" since it's the default |
There was a problem hiding this comment.
Confirm this is ok
| // results when we render the HTML page. | ||
| let contentType = response.headers.get("Content-Type"); | ||
|
|
||
| if (contentType && /\bapplication\/json\b/.test(contentType)) { |
There was a problem hiding this comment.
For the above TODO - in the router we use the following check - any concerns?
if (contentType && contentType.startsWith("application/json")) {
There was a problem hiding this comment.
Any reason to do a case insensitive check?
There was a problem hiding this comment.
Seems we do want to regex due to https://httpwg.org/specs/rfc9110.html#field.content-type
| @@ -23,7 +27,7 @@ export interface RouteModule { | |||
| | V1_HtmlMetaDescriptor | |||
| | V2_MetaFunction | |||
| | V2_HtmlMetaDescriptor[]; | |||
| unstable_shouldReload?: ShouldReloadFunction; | |||
| shouldRevalidate?: ShouldRevalidateFunction; | |||
There was a problem hiding this comment.
We are not keeping unstable_shouldReload - users will have to change to shouldRevalidate
| } | ||
| return true; | ||
| return arg.defaultShouldRevalidate; |
There was a problem hiding this comment.
shouldRevalidate replaces unstable_shouldReload
There was a problem hiding this comment.
doesn't break your app - it de-optimizes your app
| // TODO: Should we `return null` here like we do for loaders? Is there | ||
| // a benefit to triggering a network request that will error/404? |
There was a problem hiding this comment.
Confirm behavior here that we want to make the fetch request that we know is going to fail with a 405. Easier for debugging?
There was a problem hiding this comment.
No need to call the fetch for the 405 here can short circuit like we do for loaders
|
|
||
| return action; | ||
| function getRedirect(response: Response): Response { | ||
| let status = parseInt(response.headers.get("X-Remix-Status")!, 10) || 302; |
There was a problem hiding this comment.
This is new behavior to support the bug fix in the router that respects 307/308s and preserves the method/body on the redirect: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/307
Remix was previously not sending the status code along at all because it became a 204
| @@ -88,8 +88,6 @@ export function createStaticHandlerDataRoutes( | |||
| }) | |||
| : undefined, | |||
| handle: route.module.handle, | |||
| // TODO: RRR - Implement! | |||
| shouldRevalidate: () => true, | |||
There was a problem hiding this comment.
Not needed on the server routes
| // TODO Do we want to log this here? | ||
| // TODO Do we want to log this here? Can we get it to not log on | ||
| // integration tests runs? | ||
| console.log("Error in entry.server handleDocumentRequest:", error); |
There was a problem hiding this comment.
This is probably fine, can remove the todo
Co-authored-by: Jacob Ebey <jacob.ebey@live.com>
Initial work towards step 3 of React Router-ing Remix
createBrowserRouter/RouterProviderSibling RR PR: remix-run/react-router#9664
For local app testing:
latestinto your local remix apprm -rf node_modules/@remix-run/server-runtime/node_modulesrm -rf node_modules/@remix-run/react/node_modulesrm -rf node_modules/react-router-dom/node_modulesrm -rf node_modules/react-router/node_modulesREMIX_LOCAL_BUILD_DIRECTORY=../path/to/your/app yarn buildLOCAL_BUILD_DIRECTORY=../path/to/your/app yarn buildnpm run dev