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

Fog of War #9600

Merged
merged 42 commits into from
Jun 14, 2024
Merged

Fog of War #9600

merged 42 commits into from
Jun 14, 2024

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Jun 10, 2024

Implements Fog of War (remix-run/react-router#11113) for Remix via the manifest

  • The minimal __remixManifest is sent up on page load in an inline <script> just like __remixContext
  • Rendered <Link> components are batched up and a request is sent to the Remix server handler via a built-in /__manifest endpoint to match paths on the server and send back relevant route manifest entries
  • Those entries are patched into __remixManifest and the client side router
  • If a "discover" pass isn't able to patch the manifest in-time and a user clicks a Link, we fall into the patchRoutesOnMiss handler which will load patches for that path during the loading phase

RR Sibling PR: remix-run/react-router#11626
Closes #8423

Copy link

changeset-bot bot commented Jun 10, 2024

🦋 Changeset detected

Latest commit: 287cb89

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

Comment on lines +223 to +225
data-discover={
!isAbsolute && discover === "render" ? "true" : undefined
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

data-discover opts into the manifest patching. It's enabled by default (discover="render"), users can opt out via <Link discover="click">/<NavLink discover="click">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use of a data- attribute here instead of just react state is for a few reasons:

  1. Makes it easier to detect via MutationObserver and not a mess of React context/effects where Links "register" themselves with some parent context
  2. It scales very well into an RSC world where you may just want to render <a href=""> and not have to use a client component just to participate in fog of war
  3. If we ever remove Link in favor of event delegation, it works there too

@brophdawg11 brophdawg11 marked this pull request as ready for review June 13, 2024 18:17
Comment on lines -909 to -926
let nextMatches = React.useMemo(() => {
if (navigation.location) {
// FIXME: can probably use transitionManager `nextMatches`
let matches = matchRoutes(
router.routes,
navigation.location,
router.basename
);
invariant(
matches,
`No routes match path "${navigation.location.pathname}"`
);
return matches;
}

return [];
}, [navigation.location, router.routes, router.basename]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacob-ebey This seems like dead code to me? It only returns a non-empty array when we're in the middle of a client-side navigation (which implies we've already hydrated), but the routePreloads it gets concatenated into below is only ever rendered when we're not hydrated. I think it's likely leftover from the TransitionManager implementation and maybe before <PrefetchPageLinks>?

Comment on lines +151 to +154
let matches = matchServerRoutes(routes, url.pathname, _build.basename);
if (matches && matches.length > 0) {
Object.assign(params, matches[0].params);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this down so we don't bother matching on /__manifest requests

Comment on lines +138 to +152
let observer = new MutationObserver((records) => {
records.forEach((r) => {
[r.target, ...r.addedNodes].forEach((node) => {
if (!isElement(node)) return;
if (node.tagName === "A" && node.getAttribute("data-discover")) {
registerPath(node.getAttribute("href"));
} else if (node.tagName !== "A") {
node
.querySelectorAll("a[data-discover]")
.forEach((el) => registerPath(el.getAttribute("href")));
}
debouncedFetchPatches();
});
});
});
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 logic detects newly rendered links on navigations and internal route state changes (i.e., click this button to show a link) and batches them up via the debounced fetch

.querySelectorAll("a[data-discover]")
.forEach((a) => registerPath(a.getAttribute("href")));

fetchPatches();
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 does the initial fetch for all routes rendered in the initial HTML payload

Comment on lines +57 to +70
patchRoutesOnMiss: async ({ path, patch }) => {
if (fogOfWar!.known404Paths.has(path)) return;
await fetchAndApplyManifestPatches(
[path],
fogOfWar!.known404Paths,
manifest,
routeModules,
future,
isSpaMode,
basename,
patch
);
},
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we click a link that doesn't match, try to fetch that path from the server during the loading phase. In an ideal scenario, this never runs because the pre-discovery logic all lands prior to link clicks.

Comment on lines +102 to +104
if (lazyPaths.length === 0) {
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.

Short circuit there are no actively rendered paths to fetch - which means that they are all either matchable via the currently known routes, or they are a known 404 from a prior fetch.

@brophdawg11 brophdawg11 merged commit 7b7a4f9 into dev Jun 14, 2024
9 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/fog-of-war branch June 14, 2024 19:30
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Jun 14, 2024
Copy link
Contributor

🤖 Hello there,

We just published version 2.10.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!

Copy link
Contributor

🤖 Hello there,

We just published version 2.10.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!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🗺️ Fog of War
1 participant