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

Bump react-router-dom to ^6.0.1 #29553

Merged
merged 16 commits into from
Dec 1, 2021
Merged

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Nov 7, 2021

WhiteSource Renovate

This PR contains the following updates:

Package Change Age Adoption Passing Confidence
react-router-dom ^5.3.0 -> ^6.0.1 age adoption passing confidence

Release Notes

remix-run/react-router

v6.0.1

Compare Source

🐛 Bug Fixes

  • Add a default <StaticRouter location> value (#​8243)
  • Add invariant for using <Route> inside <Routes> to help people make the change (#​8238)

v6.0.0

Compare Source

React Router v6 is here!

Please go read our blog post for more information on all the great stuff in v6 including notes about how to upgrade from React Router v5 and Reach Router.


Configuration

📅 Schedule: "on sunday before 6:00am" in timezone UTC.

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

Rebasing: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 Ignore: Close this PR and you won't be reminded about these updates again.


  • If you want to rebase/retry this PR, click this checkbox.

This PR has been generated by WhiteSource Renovate. View repository job log here.

@renovate renovate bot added the dependencies Update of dependencies label Nov 7, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 7, 2021

No bundle size changes

Generated by 🚫 dangerJS against 3420a2c

@eps1lon eps1lon self-assigned this Nov 7, 2021
@eps1lon eps1lon removed their assignment Nov 8, 2021
@eps1lon
Copy link
Member

eps1lon commented Nov 8, 2021

Pretty involved migration. Take care

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 8, 2021
@michaldudak michaldudak self-assigned this Nov 18, 2021
@michaldudak
Copy link
Member

Some of the regression tests were failing because of nested routers. In react-router v6 it is not allowed to have these. The problem in our tests appeared because the test runner uses a router internally and some of the fixtures have a router inside them.
I removed the react-router components from the runner, replacing them with a very simple home-baked router. Seems to be enough for our needs.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 19, 2021
@eps1lon
Copy link
Member

eps1lon commented Nov 19, 2021

Gotcha. Can understand the rationale though I don't think it should necessarily apply to nesting different Router implementations (e.g. BrowserRouter > MemoryRouter should be fine but BrowserRouter > BrowserRouter is probably bad).

Instead, we should use different react roots alltogether. Because a router isn't trivial to implement at all. The implementation you have is not sufficient. For example, it doesn't implement correct "go back" navigation

@michaldudak
Copy link
Member

I don't think it should necessarily apply to nesting different Router implementations (e.g. BrowserRouter > MemoryRouter should be fine but BrowserRouter > BrowserRouter is probably bad)

It has been raised (remix-run/react-router#7375) but it doesn't look they're going to allow it.

The implementation you have is not sufficient. For example, it doesn't implement correct "go back" navigation

Sure thing, my intention was not to write a replacement of a react-router in 20 minutes. I figured it'll be just a bare minimum good enough for running the test suite. Is the Back button support important enough in your opinion to justify introducing another package?

@eps1lon
Copy link
Member

eps1lon commented Nov 19, 2021

Is the Back button support important enough in your opinion to justify introducing another package?

Platform feature parity is important, yes. Though we don't need another package. We can just render the TestViewer in a different root so that contexts don't clash.

This is better for isolation anyway.

@michaldudak
Copy link
Member

@eps1lon do you mean something along these lines: 29e1ba6 ?
It requires an additional 'something' (PathWatcher in this case) to detect pathname changes as I can't use useLocation outside of the Router context.

@eps1lon
Copy link
Member

eps1lon commented Nov 22, 2021

We can just render the TestViewer in a different root so that contexts don't clash.

I wasn't very clear what I mean with "root" here. I was specifically talking about a React root i.e. the ReactDOM.render() (or ReactDOM.createRoot in React 18).

So instead of rendering the fixture in the same root as the App (return children), we render it in a new React root.

function RenderIntoNewRoot({ children }) {
	React.useLayoutEffect(() => {
    ReactDOM.render(children, differentContainerYouGetFromSomewhere)
  }, [children])

  React.useLayoutEffect(() => {
    return () => {
      ReactDOM.unmountComponentAtNode(differentContainerYouGetFromSomewhere)
    }
  }, [])

  
  return null;
}

@michaldudak
Copy link
Member

@eps1lon
Copy link
Member

eps1lon commented Nov 22, 2021

@eps1lon could you please take a look at #29553 (files) if this is what you had in mind?

Yep, perfect. I originally though about creating the root into the TestViewer. But index.js is the better location since TestViewer should assume that it can "just render children". But index.js actually knows where the children are coming from and therefore knows about the router issue not TestViewer.

@michaldudak
Copy link
Member

@eps1lon Thanks for your help on this.
I haven't found anything that breaks on the site after all these changes. Would you like to take one more look at the code or are we good to merge?

@michaldudak michaldudak merged commit 38ac830 into master Dec 1, 2021
@michaldudak michaldudak deleted the renovate/react-router-dom-6.x branch December 1, 2021 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants