remix
remix copied to clipboard
perf(remix-dev): Optimize parentRouteId lookup in defineConventionalRoutes
Closes: #4480. defineConventionalRoutes was repeatedly running .filter and .find against the list of routeIds to find parents, which was causing an exponential slowdown in larger projects. This update first generates a parentRouteIds object and then filters against that instead.
- [ ] Docs
- [x] Tests
Testing Strategy: There weren't any direct tests for defineConventionalRoutes as far as I could tell, so I added one to make sure the necessary number of routes (including parent route ids) are generated. I used an existing fixture for the routes folders (remix-dev/__tests__/fixtures/replace-remix-imports) but I can add one specific to the routesConventions tests if you prefer this test remain uncoupled from the replace imports test.
I ran this against my production app with nearly 700 routes and the RouteManifest object was was identical before and after the patch. Before this update, my production app was taking around 10-15 seconds before "Watching Remix app in development mode..." showed up, now it's less than a second.
I also ran the example test app from my original issue against this new code and the speed up is drastic on large projects (almost 27 seconds before for 1,111 routes, now it's 0.104 seconds).
$ node run.js 1 | head
Creating sample routes...
defineConventionalRoutes: 0.008s for 112 routes (0.07ms per file)
<Routes>
<Route file="root.tsx">
<Route path="0/10/0/0" index file="routes/__a/__b/__c/0/10/0/0/index.js" />
<Route path="0/10/0/1" index file="routes/__a/__b/__c/0/10/0/1/index.js" />
<Route path="0/10/1/0" index file="routes/__a/__b/__c/0/10/1/0/index.js" />
<Route path="0/10/1/1" index file="routes/__a/__b/__c/0/10/1/1/index.js" />
<Route path="0/10/2/0" index file="routes/__a/__b/__c/0/10/2/0/index.js" />
<Route path="0/10/2/1" index file="routes/__a/__b/__c/0/10/2/1/index.js" />
$ node run.js 5 | head
Creating sample routes...
defineConventionalRoutes: 0.044s for 556 routes (0.08ms per file)
<Routes>
<Route file="root.tsx">
<Route path="0/10/0/0" index file="routes/__a/__b/__c/0/10/0/0/index.js" />
<Route path="0/10/0/1" index file="routes/__a/__b/__c/0/10/0/1/index.js" />
<Route path="0/10/1/0" index file="routes/__a/__b/__c/0/10/1/0/index.js" />
<Route path="0/10/1/1" index file="routes/__a/__b/__c/0/10/1/1/index.js" />
<Route path="0/10/2/0" index file="routes/__a/__b/__c/0/10/2/0/index.js" />
<Route path="0/10/2/1" index file="routes/__a/__b/__c/0/10/2/1/index.js" />
$ node run.js 10 | head
Creating sample routes...
defineConventionalRoutes: 0.104s for 1111 routes (0.09ms per file)
<Routes>
<Route file="root.tsx">
<Route path="0/10/0/0" index file="routes/__a/__b/__c/0/10/0/0/index.js" />
<Route path="0/10/0/1" index file="routes/__a/__b/__c/0/10/0/1/index.js" />
<Route path="0/10/1/0" index file="routes/__a/__b/__c/0/10/1/0/index.js" />
<Route path="0/10/1/1" index file="routes/__a/__b/__c/0/10/1/1/index.js" />
<Route path="0/10/2/0" index file="routes/__a/__b/__c/0/10/2/0/index.js" />
<Route path="0/10/2/1" index file="routes/__a/__b/__c/0/10/2/1/index.js" />
⚠️ No Changeset found
Latest commit: d7087025953e5c70a9793c8c05bb69448980c945
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
Hi @dmarkow,
Welcome, and thank you for contributing to Remix!
Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.
You may review the CLA and sign it by adding your name to contributors.yml.
Once the CLA is signed, the CLA Signed label will be added to the pull request.
If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].
Thanks!
- The Remix team
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳
Hey @dmarkow and thank you for this PR!
Could you rebase it against the dev branch? This is where code modifications should go.
@machour My git rebasing skills are apparently trash 😅 Rebasing my local branch against dev is still keeping all of main's recent commits as well, and changing the base branch of this PR causes 11 different commits from main to pull over.
Can you point me in the right direction on this, or should I just start over and push a new PR originating from the dev branch?
@machour Nevermind, figured out rebase --onto right after I replied. It should be ready to go now.
@pcattori Just fixed the tests (looks like an existing fixture changed paths)
@dmarkow thanks for your work! Should make a HUGE difference to many Remix users ❤️