Skip to content

Commit

Permalink
fix: <Link prefetch> fetching too many things
Browse files Browse the repository at this point in the history
- ensure splat param changes are localized to the match, before we were prefetching parents of splat routes all the time
- accounted for `shouldReload` with data prefetching on search param changes
  • Loading branch information
ryanflorence committed Oct 29, 2021
1 parent f896ac3 commit bdd983c
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 45 deletions.
19 changes: 12 additions & 7 deletions packages/remix-react/components.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -563,24 +563,29 @@ function PrefetchPageLinksImpl({
let location = useLocation();
let { matches, manifest } = useRemixEntryContext();

let newMatches = React.useMemo(
() => getNewMatchesForLinks(page, nextMatches, matches, location),
let newMatchesForData = React.useMemo(
() => getNewMatchesForLinks(page, nextMatches, matches, location, "data"),
[page, nextMatches, matches, location]
);

let newMatchesForAssets = React.useMemo(
() => getNewMatchesForLinks(page, nextMatches, matches, location, "assets"),
[page, nextMatches, matches, location]
);

let dataHrefs = React.useMemo(
() => getDataLinkHrefs(page, newMatches, manifest),
[newMatches, page, manifest]
() => getDataLinkHrefs(page, newMatchesForData, manifest),
[newMatchesForData, page, manifest]
);

let moduleHrefs = React.useMemo(
() => getModuleLinkHrefs(newMatches, manifest),
[newMatches, manifest]
() => getModuleLinkHrefs(newMatchesForAssets, manifest),
[newMatchesForAssets, manifest]
);

// needs to be a hook with async behavior because we need the modules, not
// just the manifest like the other links in here.
let styleLinks = usePrefetchedStylesheets(newMatches);
let styleLinks = usePrefetchedStylesheets(newMatchesForAssets);

return (
<>
Expand Down
61 changes: 49 additions & 12 deletions packages/remix-react/links.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,28 +266,65 @@ export async function getStylesheetPrefetchLinks(
.map(({ rel, ...attrs }) => ({ rel: "prefetch", as: "style", ...attrs }));
}

// This is ridiculously identical to transition.ts `filterMatchesToLoad`
export function getNewMatchesForLinks(
page: string,
nextMatches: RouteMatch<ClientRoute>[],
currentMatches: RouteMatch<ClientRoute>[],
location: Location
location: Location,
mode: "data" | "assets"
): RouteMatch<ClientRoute>[] {
let path = parsePathPatch(page);

let isNew = (match: RouteMatch<ClientRoute>, index: number) => {
if (!currentMatches[index]) return true;
return match.route.id !== currentMatches[index].route.id;
};

let matchPathChanged = (match: RouteMatch<ClientRoute>, index: number) => {
return (
// param change, /users/123 -> /users/456
currentMatches[index].pathname !== match.pathname ||
// splat param changed, which is not present in match.path
// e.g. /files/images/avatar.jpg -> files/finances.xls
(currentMatches[index].route.path?.endsWith("*") &&
currentMatches[index].params["*"] !== match.params["*"])
);
};

// NOTE: keep this mostly up-to-date w/ the transition data diff, but this
// version doesn't care about submissions
let newMatches =
location.search !== path.search
? nextMatches
: nextMatches.filter(
(nextMatch, index) =>
// new route
!currentMatches[index] ||
// existing route but params changed
currentMatches[index].pathname !== nextMatch.pathname ||
// catchall param changed
currentMatches[index].params["*"] !== nextMatch.params["*"]
);
mode === "data" && location.search !== path.search
? // this is really similar to stuff in transition.ts, maybe somebody smarter
// than me (or in less of a hurry) can share some of it. You're the best.
nextMatches.filter((match, index) => {
if (!match.route.hasLoader) {
return false;
}

if (isNew(match, index) || matchPathChanged(match, index)) {
return true;
}

if (match.route.shouldReload) {
return match.route.shouldReload({
params: match.params,
prevUrl: new URL(
location.pathname + location.search + location.hash,
window.origin
),
url: new URL(page, window.origin)
});
}
return true;
})
: nextMatches.filter((match, index) => {
return (
match.route.hasLoader &&
(isNew(match, index) || matchPathChanged(match, index))
);
});

return newMatches;
}
Expand Down
4 changes: 3 additions & 1 deletion packages/remix-react/routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export interface ClientRoute extends Route {
children?: ClientRoute[];
element: ReactNode;
module: string;
hasLoader: boolean;
}

type RemixRouteComponentType = ComponentType<{ id: string }>;
Expand All @@ -92,7 +93,8 @@ export function createClientRoute(
action: createAction(entryRoute),
shouldReload: createShouldReload(entryRoute, routeModulesCache),
ErrorBoundary: entryRoute.hasErrorBoundary,
CatchBoundary: entryRoute.hasCatchBoundary
CatchBoundary: entryRoute.hasCatchBoundary,
hasLoader: entryRoute.hasLoader
};
}

Expand Down
28 changes: 3 additions & 25 deletions packages/remix-react/transition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1268,7 +1268,8 @@ function filterMatchesToLoad(
state.matches[index].pathname !== match.pathname ||
// splat param changed, which is not present in match.path
// e.g. /files/images/avatar.jpg -> files/finances.xls
state.matches[index].params["*"] !== match.params["*"]
(state.matches[index].route.path?.endsWith("*") &&
state.matches[index].params["*"] !== match.params["*"])
);
};

Expand Down Expand Up @@ -1310,30 +1311,7 @@ function filterMatchesToLoad(
// search affects all loaders
url.searchParams.toString() !== state.location.search
) {
return matches.filter((match, index) => {
if (!match.route.loader) {
return false;
}

// you don't get a choice when you're new or your params changed
if (isNew(match, index) || matchPathChanged(match, index)) {
return true;
}

// but if you were already on the page and your path didn't change, apps
// get a chance to optimize
if (match.route.shouldReload) {
let prevUrl = createUrl(createHref(state.location));
return match.route.shouldReload({
prevUrl,
url,
submission,
params: match.params
});
}

return true;
});
return matches.filter(filterByRouteProps);
}

return matches.filter((match, index, arr) => {
Expand Down

0 comments on commit bdd983c

Please sign in to comment.