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

Add future.v7_relativeSplatPath flag #11087

Merged
merged 9 commits into from
Dec 5, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Dec 4, 2023

Re-implement the bug fix from #10983 behind a future.v7_relativeSplatPath flag because we determined that a lot of folks were relying on the buggy behavior (see #11052 and #11078).

Usage:

/**** Data router ****/
let router = createBrowserRouter(routes, { 
  future: { v7_relativeSplatPath: true }
});
<RouterProvider router={router} />

/**** Non-Data Router ****/
<BrowserRouter future={{ v7_relativeSplatPath: true }}>

/**** Data Router SSR ****/
let { query, dataRoutes } = createStaticHandler(routes, {
  future: { v7_relativeSplatPath: true },
});
let router = createStaticRouter(dataRoutes, context, {
  future: { v7_relativeSplatPath: true },
});
<StaticRouterProvider router={router} context={context} />

/**** Non-Data Router SSR ****/
<StaticRouter future={{ v7_relativeSplatPath: true }}>

Todo:

  • Docs + examples of buggy behavior

This commit started with an initial revert of commit 9cfd99d
Copy link

changeset-bot bot commented Dec 4, 2023

🦋 Changeset detected

Latest commit: 576be9e

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

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

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

// https://github.com/remix-run/react-router/issues/11052#issuecomment-1836589329
if (v7_relativeSplatPath) {
return pathMatches.map((match, idx) =>
idx === matches.length - 1 ? match.pathname : match.pathnameBase
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the primary functional code change - when the flag is enabled we leverage pathname on the leaf match to ensure we include the splat portion of the path

let { matches } = React.useContext(RouteContext);
let { pathname: locationPathname } = useLocation();

let routePathnamesJson = JSON.stringify(
getPathContributingMatches(matches).map((match) => match.pathnameBase)
getResolveToMatches(matches, future.v7_relativeSplatPath)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everywhere we used to do this pathnameBase mapping is now done via getResolveToMatches

@brophdawg11 brophdawg11 merged commit 149ad65 into dev Dec 5, 2023
3 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/resolve-path-splat-flag branch December 5, 2023 16:35
Copy link
Contributor

github-actions bot commented Dec 5, 2023

🤖 Hello there,

We just published version 6.21.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@cduff
Copy link

cduff commented Dec 8, 2023

@brophdawg11

I first asked this question over at #11052 but got no response, so thought I'd try here.

I was hit by #11052 and was just thinking about how I came to rely on this "buggy behavior".

I originally used Reach Router, as recommended by the React Router team at the time (before React Router v6).

I used "Embedded Routers" as per the example on this page: https://reach.tech/router/nesting.

I then migrated to React Router as recommended by the team.

Is my understanding correct that the Reach Router "Embedded Routers" example is now considered to have relied on buggy behavior?

Just trying to understand what the future direction might be and how it might impact my app.

Thanks for your time.

@brophdawg11
Copy link
Contributor Author

Yes - the behavior was chosen partially to help support the concept of embedded routers, but did not foresee the other inconsistencies that were introduced by that choice - such that . no longer meant the current location and path resolution would work inconsistently within path="dashboard/*" and path="dashboard/:page" etc. We wrote up some of the inconsistencies in the changeset and the useResolvedPath docs (currently available on the release-next branch)

Copy link
Contributor

🤖 Hello there,

We just published version 6.21.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@jasikpark
Copy link
Contributor

jasikpark commented Dec 13, 2023

Should

// Only accept future flags relevant to rendering behavior
accept this future flag?

return <RouterProvider router={router} future={{ v7_relativeSplatPath: true }} />; errors for me, since this new tag is filtered out

I see I should have specified in the route config, as is in the PR description

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.

3 participants