Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Remix to use route.lazy from React Router #7133

Merged
merged 9 commits into from
Aug 22, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Aug 10, 2023

Leverage route.lazy from React Router

TODO:

@changeset-bot
Copy link

changeset-bot bot commented Aug 10, 2023

🦋 Changeset detected

Latest commit: 67fa0f3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/react Minor
@remix-run/server-runtime Minor
@remix-run/testing Minor
@remix-run/cloudflare Minor
@remix-run/deno Minor
@remix-run/dev Minor
@remix-run/node Minor
@remix-run/cloudflare-pages Minor
@remix-run/cloudflare-workers Minor
@remix-run/architect Minor
@remix-run/express Minor
@remix-run/serve Minor
create-remix Minor
remix Minor
@remix-run/css-bundle Minor
@remix-run/eslint-config Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@brophdawg11 brophdawg11 added the v2 Issues related to v2 apis label Aug 16, 2023
@brophdawg11 brophdawg11 self-assigned this Aug 16, 2023
window.location.reload();
// Get out of here so the reload can happen - don't create the router
// since it'll then kick off unnecessary route.lazy() loads
return <></>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this we get into I think the same hydration error loop as #1678. I hope to dig deeper into that later today/tomorrow so maybe it'll shed some light on why we need the short circuit, but once I move this up and return (and avoid any router initialization + lazy calls on the wrong route) the E2E test passes and all of my manual testing with rapid forward clicks into a Remix app to cause a mismatched URL at hydration time settle correctly on the final URL after the hard reload.

////////////////////////////////////////////////////////////////////////////////
// RemixRoute

export function RemixRoute({ id }: { id: string }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

RemixRoute and RemixRouteError were sort of poor-mans implementations of Component/ErrorBoundary lazy loading. We could assign them to a route definition up front (without a route module available) knowing that they'd pull from the lazily loaded routeModulesCache at render-time. Now we can just assign Component/ErrorBoundary via lazy().

The only downside is we lose these error messages - so if we think they are worth keeping we can stick with this approach.

Comment on lines -1064 to -1066
// Need to grab handle here since we don't have it at client-side route
// creation time
handle: routeModules[match.id].handle,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer need to grab handle from the routeModulesCache since it's populated via lazy()

Comment on lines +133 to +147
loader({ request }) {
if (!route.hasLoader) return null;
return fetchServerHandler(request, route);
},
action({ request }) {
if (!route.hasAction) {
let msg =
`Route "${route.id}" does not have an action, but you are trying ` +
`to submit to it. To fix this, please add an \`action\` function to the route`;
console.error(msg);
return Promise.reject(new Error(msg));
}

return fetchServerHandler(request, route);
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lifted the forked logic from createDataFunction to inline here so we can share 100% of fetchServerHandler

Comment on lines +148 to +182
...(routeModule
? // Use critical path modules directly
{
Component: routeModule.default,
ErrorBoundary: routeModule.ErrorBoundary
? routeModule.ErrorBoundary
: route.id === "root"
? () => <RemixRootDefaultErrorBoundary error={useRouteError()} />
: undefined,
handle: routeModule.handle,
shouldRevalidate: needsRevalidation
? wrapShouldRevalidateForHdr(
route.id,
routeModule.shouldRevalidate,
needsRevalidation
)
: routeModule.shouldRevalidate,
}
: // Load all other modules via route.lazy()
{
lazy: async () => {
let mod = await loadRouteModuleWithBlockingLinks(
route,
routeModulesCache
);
if (needsRevalidation) {
mod.shouldRevalidate = wrapShouldRevalidateForHdr(
route.id,
mod.shouldRevalidate,
needsRevalidation
);
}
return mod;
},
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the meat of this PR - non-critical routes get everything populated via lazy now

@brophdawg11 brophdawg11 marked this pull request as ready for review August 17, 2023 16:25
@brophdawg11 brophdawg11 merged commit 25e5d32 into dev Aug 22, 2023
9 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/route-lazy branch August 22, 2023 19:14
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Aug 22, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-49e8da1-20230823 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue has been fixed and will be released soon CLA Signed feat:routing renderer:react v2 Issues related to v2 apis
Projects
No open projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

3 participants