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

perf(remix-dev): Optimize parentRouteId lookup in defineConventionalRoutes #4538

Merged
merged 4 commits into from
Dec 7, 2022

Conversation

dmarkow
Copy link
Contributor

@dmarkow dmarkow commented Nov 7, 2022

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
  • 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" />

@changeset-bot
Copy link

changeset-bot bot commented Nov 7, 2022

⚠️ No Changeset found

Latest commit: d708702

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Nov 7, 2022

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 hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Nov 7, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@machour machour changed the title remix-dev routesConvention: Use object for parentRouteId lookup perf(remix-dev): Optimize parentRouteId lookup in defineConventionalRoutes Nov 7, 2022
@machour
Copy link
Collaborator

machour commented Nov 7, 2022

Hey @dmarkow and thank you for this PR!

Could you rebase it against the dev branch? This is where code modifications should go.

@machour machour added needs-response We need a response from the original author about this issue/PR package:dev labels Nov 7, 2022
@dmarkow dmarkow changed the base branch from main to dev November 7, 2022 14:47
@dmarkow dmarkow changed the base branch from dev to main November 7, 2022 14:47
@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 7, 2022

@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?

@dmarkow dmarkow changed the base branch from main to dev November 7, 2022 14:52
@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 7, 2022

@machour Nevermind, figured out rebase --onto right after I replied. It should be ready to go now.

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Nov 7, 2022
@MichaelDeBoey MichaelDeBoey changed the title perf(remix-dev): Optimize parentRouteId lookup in defineConventionalRoutes perf(remix-dev): Optimize parentRouteId lookup in defineConventionalRoutes Dec 7, 2022
Copy link
Contributor

@pcattori pcattori left a comment

Choose a reason for hiding this comment

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

LGTM

@dmarkow looks like some tests are failing. Could you rebase on latest dev and see if that fixes things?

@dmarkow
Copy link
Contributor Author

dmarkow commented Dec 7, 2022

@pcattori Just fixed the tests (looks like an existing fixture changed paths)

@pcattori pcattori merged commit 18295fd into remix-run:dev Dec 7, 2022
@pcattori
Copy link
Contributor

pcattori commented Dec 7, 2022

@dmarkow thanks for your work! Should make a HUGE difference to many Remix users ❤️

pcattori added a commit that referenced this pull request Dec 7, 2022
pcattori added a commit that referenced this pull request Dec 7, 2022
pcattori added a commit that referenced this pull request Dec 16, 2022
* fix(remix-dev): convert `config.appDirectory` to relative unix path (#4709)

* fix(remix-dev): convert appDirectory to unix style for fast-glob

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: relative path

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: add test

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* fix test

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* fix: typo

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: update test

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: add changeset for #4709 (#4718)

* ci: add typechecking for deno (#4715)

* ci: add typechecking for deno

* ci: install deno for integration tests

* chore: format

* chore: format

* fix: Firefox LiveReload (#4725)

Firefox infinitely reloads the page as long as `<LiveReload>` is rendering.

Closes #4692

* fix(remix-dev): allow defining multiple routes for the same route module file (#3970)

* Allow multiple routes for same route module

* Update packages/remix-dev/config/routes.ts

Co-authored-by: Andrew Leedham <AndrewLeedham@outlook.com>

* Update routes.ts

- Better name for automated ID variable;
- Small adjust in `id` option comment;

* - Removing redundant IF

* Update routes.ts

Revert complex custom ID in routes

* Non unique custom routes ID error and test

* Update assets.ts

Trying to solve a conflict

* Revert "Update assets.ts"

This reverts commit 2064c57.

* Error on collisions with non-custom routeIds

* Create big-spoons-grab.md

Co-authored-by: Andrew Leedham <AndrewLeedham@outlook.com>
Co-authored-by: Matt Brophy <matt@brophy.org>

* feat: Allow pass-through script props in `ScrollRestoration` (#2879)

* ci(nightly): add deno for typechecking deno files (#4738)

* ci: fix race condition writing globals.d.ts shim (#4717)

Co-authored-by: Chance Strickland <hi@chance.dev>

* chore: bump @playwright/test to latest (#4749)

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* Fix 4199: TypedResponse allows incompatible types (#4734)

* Fixes #4199: Do not allow assignment of incompatible TypedResponses

* Add myself to contributors.yml

* Create light-sheep-give.md

* slight changeset tweak

* additional changeset tweaks

Co-authored-by: Pedro Cattori <pcattori@gmail.com>

* chore: format

* test: add transition integration tests (#4739)

test: useTransition to wait for states

This approach could probably be applied across other flakey tests and could also be documented as a good approach of if there is user-feedback for a specific action when running integration tests.

* feat: testing helpers (#4539)

Co-authored-by: James Restall <james.restall@gmail.com>
Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: format

* chore(remix-testing): update dependencies (#4756)

* fix(remix-testing): fix deps (#4757)

* Fix deps for new remix-testing package

* fix lint

* Remove ENABLE_REMIX_ROUTER flags (#4732)

* chore: add `@remix-run/testing` to changesets (#4781)

* chore: add `@remix-run/testing` to changesets

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: update `ADDING_A_PACKAGE.md`

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* refactor(remix-react): upgrade Remix to `react-router-dom@6.4` (non-data-router) and drop `history` (#4731)

* Bump remix to react-router-dom@6.4.4 (#4668)

Co-authored-by: Mehdi Achour <machour@gmail.com>

* Bump remix to RR 6.4.4 and drop history dependency (#4702)

Co-authored-by: Mehdi Achour <machour@gmail.com>

* ci(nightly): move git operations after build (#4797)

* ci(nightly): move git operations after build

when adding deno typechecking (#4715), nightly now refers to the version we just calculated which hasnt been published when trying to build and typecheck

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* ci: update tmp branch name

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* perf(remix-dev): Optimize `parentRouteId` lookup in `defineConventionalRoutes` (#4538)

* Use object for parentRouteId lookup

* Sign CLA

* Move parentRouteId logic to a separate function

* Update test fixture path

* chore: format

* chore(dev): add changeset for PR #4538 (#4800)

* chore: format

* refactor(remix-react): use `react-router-dom` import instead of `react-router` (#3325)

* chore: manually bump remix-testing version due to not being on main just yet

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* fixup! chore: manually bump remix-testing version due to not being on main just yet

* fixup! chore: manually bump remix-testing version due to not being on main just yet

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: unify error usage (#4696)

* chore: format

* Add fetcher state/type tests (#4803)

* chore(deps): bump esbuild to latest (#4754)

* chore: add invariant instead of using `!`

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore(deps): bump esbuild to latest

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* Create fresh-shrimps-join.md

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Pedro Cattori <pcattori@gmail.com>

* test(integration): close server synchronously  (#4785)

* chore: normalize afterAll

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* test: close server synchronously

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: appFixture.close isnt async anymore

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* Update integration/helpers/create-fixture.ts

Co-authored-by: Pedro Cattori <pcattori@gmail.com>

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Pedro Cattori <pcattori@gmail.com>

* feat: remix optional segments (#4706)

* feat: transform optional routes from remix to react router

* Add to contributors

* Add changeset

* fix(optional-segments): fix escaping of parenthesis

* small function fix

* Update packages/remix-dev/__tests__/routesConvention-test.ts

Co-authored-by: Pedro Cattori <pcattori@gmail.com>

Co-authored-by: Pedro Cattori <pcattori@gmail.com>

* chore: format

* chore: edit the optional segments changeset (#4815)

to make it clear that Remix won't support optional segments until integrated with React Router 6.5

* chore: format

* docs: rearrange docs, give everything its own page (#4821)

* chore: format

* fix: wrong parentheses in the optional segments changeset (#4825)

* fix: wrong parentheses in the optional segments changeset

* add: akamfoad to the contributers

* ci(nightly): add workflow_call for testing purposes

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* Revert "ci(nightly): add workflow_call for testing purposes"

This reverts commit f3fa77e.

* chore(scripts): Use relaxed peer dependencies to avoid triggering major version bumps (#4736)

- Add script to bump peer deps with changesets version

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Add integration tests for request structures (#4829)

* fix(remix-dev): resolve asset entry full path to support mono-repo import of styles (#4855)

* chore: have eslint report unused eslint comments (#4863)

* chore: have eslint report unused eslint comments

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: remove additional comment

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* perf(remix-architect,remix-netlify): improve performance of `isBinaryType` (#4761)

Co-authored-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Pannatier Guillaume <Guillaume.Pannatier@hopitalvs.ch>

* chore: format

* chore(remix-testing): remove internal installGlobals (#4755)

* chore: remove internal installGlobals

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: add README

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* Create quick-cats-fix.md

* Update .changeset/quick-cats-fix.md

Co-authored-by: Michaël De Boey <info@michaeldeboey.be>

* chore(deps): remove jsdom and happydom from devDependencies

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>

* fix(dev): build js modules for ts->js conversion

The TS->JS migration was removed from the CLI codemod options, but still
used for TS->JS conversion when creating a new Remix project from the
CLI. The TS modules responsible for the TS->JS conversion were
incorrectly removed from the Rollup build, resulting in the
corresponding built JS modules being absent. That caused the CLI to
error when trying to perform TS->JS conversion. This changes
reintroduces the wiring to build the modules responsible for the TS->JS
conversion.

Fixes #4854

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Matt Kane <m@mk.gg>
Co-authored-by: Remix Run Bot <hello@remix.run>
Co-authored-by: Chance Strickland <hi@chance.dev>
Co-authored-by: Ryan Florence <rpflorence@gmail.com>
Co-authored-by: Lucas Ferreira <panchorf@gmail.com>
Co-authored-by: Andrew Leedham <AndrewLeedham@outlook.com>
Co-authored-by: Matt Brophy <matt@brophy.org>
Co-authored-by: dabdine <1955040+dabdine@users.noreply.github.com>
Co-authored-by: Jacob Ebey <jacob.ebey@live.com>
Co-authored-by: James Restall <james.restall@gmail.com>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: Mehdi Achour <machour@gmail.com>
Co-authored-by: Dylan Markow <dylan@dylanmarkow.com>
Co-authored-by: Daniel Rios <ieldanr@gmail.com>
Co-authored-by: Akam Foad <41629832+akamfoad@users.noreply.github.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Guillaume Pannatier <guillaume.pannatier@gmail.com>
Co-authored-by: Pannatier Guillaume <Guillaume.Pannatier@hopitalvs.ch>
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.

Delayed startup due to slow route generation
4 participants