Fix issue with route ordering for ranking ties #7884
Conversation
🦋 Changeset detectedLatest commit: 432fae4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
| await app.goto("/legal/apple-pay"); | ||
| await page.waitForSelector("h1"); | ||
| expect(await app.getHtml("h1")).toMatch(":locale?/apple-pay"); | ||
| expect(await app.getHtml("pre")).toMatch(":locale?/apple-pay"); |
There was a problem hiding this comment.
The tie here occurs because ($locale).apple-pay.jsx and ($locale).legal.$article.jsx explode out to the following in the react router routes:
:locale/apple-pay
/apple-pay
:locale/legal/:article
legal/:article
And then the URL /legal/apple-pay matches both of these routes with an identical score:
:locale/apple-pay
legal/:article
In this first test, we expect ($locale).apple-pay.jsx to win due to alphabetical ordering
| await app.goto("/legal/apple-pay"); | ||
| await page.waitForSelector("h1"); | ||
| expect(await app.getHtml("h1")).toMatch(":locale?/legal/:article"); | ||
| expect(await app.getHtml("pre")).toMatch(":locale?/legal/:article"); |
There was a problem hiding this comment.
In this test, we expect ($locale).legal.$article.jsx to win because we included it first in remix.config.js routes
| @@ -163,6 +164,7 @@ export function flatRoutesUniversal( | |||
| } | |||
|
|
|||
| routeIds.set(routeId, normalizedFile); | |||
| configRouteOrder.set(routeId, idx); | |||
There was a problem hiding this comment.
Capture the order of routeIds in the config so we can sort by them later
brophdawg11
changed the title
|
Huh - this used to only fail the Windows E2E tests and we didn't know why nor did we have a chance to dig too deep into what was going on there, but they pass now so I'm not 100% sure what fixed it - nor are the logs available for those CI runs anymore. I'm assuming we fixed some sort of other bug in our windows dev setup along the way... |
| let sortedRoutes = Object.keys(routes) | ||
| .sort((a, b) => (order[a] < order[b] ? -1 : order[a] > order[b] ? 1 : 0)) | ||
| .reduce((acc, id) => Object.assign(acc, { [id]: routes[id] }), {}); | ||
|
|
There was a problem hiding this comment.
Vite does not have this issue because it generates the manifest in the order it gets from remixConfig.routes so the order is already correct there after the fix to flat-routes.ts below.
|
From what I can tell this was only an issue in the esbuild compiler where the flat routes and manifest could end up in different orders causing hydration issues. With Vite - the manifest is generated from the output of the flat routes + config I think technically it still makes sense to re-sort the flat routes logic alphabetically as we do in this PR but I think that could be interpreted as a breaking change in v2. RR v7 will be moving towards |
Fix route ordering discrepancies between
build.routesandbuild.assets.routesIt's possible for the current React Router ranking algorithm to end up with ties, and in those cases the first matching route "wins". In React Router you can manage these tie breakers by the order in which you define your routes. But in Remix they are (by default) defined by the order of the route files on the file system.
The bug being fixed here was that in our flat routes logic we did some sorting by length to determine parent routes, and this would result in the eventual defined routes being sorted in the same manner (
build.routes). However, our actual manifest generation happens in the order of theesbuildmetafileoutputs (build.assets.routes).If these two ended up in different orders we could end up with out of sync route matching in the
server-runtimeand<RemixServer>. The specific collision encountered is represented in the integration tests in this PR.This PR updates the routes and manifest to always respect the original order of the incoming
config.routes, which accomplished 2 things:remix.config.jssince the config order is the source of truth.Repro: https://stackblitz.com/edit/remix-run-remix-5udtin
2.11.2and confirmed fixed by0.0.0-experimental-432fae462built from this branch