Skip to content

feat: mutable route tree #9996

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Feb 21, 2023
Merged

feat: mutable route tree #9996

merged 12 commits into from
Feb 21, 2023

Conversation

@jacob-ebey
Copy link
Member

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Jan 28, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: cf9637c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Patch
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

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

@timdorr timdorr mentioned this pull request Jan 30, 2023
Closed
@pcattori pcattori mentioned this pull request Jan 31, 2023
Closed
3 tasks
@pcattori pcattori mentioned this pull request Feb 15, 2023
Merged
19 tasks
@pcattori pcattori marked this pull request as ready for review February 15, 2023 23:31
@pcattori pcattori requested a review from brophdawg11 February 15, 2023 23:32
brophdawg11
brophdawg11 reviewed Feb 16, 2023
View reviewed changes
Copy link
Contributor

@brophdawg11 brophdawg11 left a comment

Choose a reason for hiding this comment

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

LGTM! Let's add some edge case unit tests? We have the "removed a loader during HMR", but probably want "added a loader" as well as a few around revalidation being interrupted by a navigation + fetch

packages/router/router.ts
saveScrollPosition(state.location, state.matches);
pendingPreventScrollReset = (opts && opts.preventScrollReset) === true;

let routesToUse = inFlightDataRoutes || dataRoutes;
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels sufficient for now since it's only needed in a few spots. But I am wondering if this feels like a hard-to-detect footgun down the road if we add some new code and just reach for dataRoutes. One thought was a quick getter function like getCurrentDataRoutes() but maybe even something like renaming dataRoutes -> comittedRoutes would help add some explicit semantics too. I think it's probably best to table the decision until we tackle the follow ups for fog of war and MFE though

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea let's tackle this for sure when we got to the granular route updates API

@ryanflorence ryanflorence merged commit 3eac3a6 into dev Feb 21, 2023
@ryanflorence ryanflorence deleted the mutable-route-tree branch February 21, 2023 19:41
This was referenced Feb 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pcattori pcattori pcattori approved these changes

@brophdawg11 brophdawg11 brophdawg11 approved these changes

Assignees

No one assigned

Labels

CLA Signed pkg:react-router

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jacob-ebey @pcattori @brophdawg11 @ryanflorence @MichaelDeBoey