Jetpack: update react-router-dom from v5 to v6 #40773
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
bot
added
[Plugin] Jetpack
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
| } | ||
|
|
||
| componentDidUpdate( oldprops ) { | ||
| // We need to handle the search term not only on route update but also on page load in case of some external redirects | ||
| if ( oldprops.location !== this.props.location ) { | ||
| this.onRouteChange( this.props.location ); | ||
| } |
There was a problem hiding this comment.
There's no direct upgrade path for this, but hopefully this roughly the same thing as before.
tbradsha
force-pushed
the
update/jetpack/react-router-dom_v5_to_v6
branch
from
243b30a to
93b00f6
Compare
| * @param {object} root0.location - location object | ||
| * @param {string} root0.location.pathname - location pathname |
There was a problem hiding this comment.
Instead of bc03437, it looks like a more "proper" way to fix the tests may be to put them inside a <MemoryRouter> that is initialized with the correct location paths, something like this
<MemoryRouter initialEntries={ [ '/whatever' ] }>
<PlanConflictWarning />
</MemoryRouter>Although to get it to actually work I wound up with something more like this:
/**
* Wrap the component in a `<MemoryRouter>` for testing.
*
* @param {PlanConflictWarning} component The `<PlanConflictWarning>` to wrap.
* @param {string[]} entries Initial path entries.
* @returns {MemoryRouter} Router to render..
*/
function wrapWithMemoryRouter( component, entries = [ '/plans' ] ) {
return (
<MemoryRouter
initialEntries={ entries }
future={
// Enable some forward-compat things so it doesn't console.warn about enabling them. Sigh.
{ v7_startTransition: true, v7_relativeSplatPath: true }
}
>
{ component }
</MemoryRouter>
);
}| } | ||
|
|
||
| componentDidUpdate( oldprops ) { | ||
| // We need to handle the search term not only on route update but also on page load in case of some external redirects |
There was a problem hiding this comment.
The bits here on line 45–47 are the "on route update", while the UNSAFE_componentWillMount() is the "also on page load". I think this comment may be in the wrong place?
| @@ -854,10 +857,12 @@ class Main extends React.Component { | |||
| <AdminNotices /> | |||
| <JetpackNotices /> | |||
| { this.shouldConnectUser() && this.connectUser() } | |||
| { /* This is no longer supported as of react-router-dom v6: https://github.com/remix-run/react-router/issues/8139 | |||
There was a problem hiding this comment.
If we really want it, I think we could use unstable_usePrompt(). It claims it can be weird in different browsers in some cases though.
For use inside a class component, like here, that might mean creating our own tiny <Prompt> component something like this:
function Prompt( { when, message } ) {
unstable_usePrompt( { when, message } );
return null;
}
|
I think I see why the E2Es are failing. Formerly, due to lack of jetpack/projects/plugins/jetpack/_inc/client/admin.js Lines 56 to 58 in f66d303 /recommendations. But that was ok, since jetpack/projects/plugins/jetpack/_inc/client/recommendations/index.jsx Lines 214 to 334 in f66d303 Now, though, we get the
AFAICT, to fix all this we need to make that be |
|
P.S. Something that's really confusing is that v6 allows absolute paths in nested routes for something like <Routes>
<Route path="/foo/*" element={<Foo />}>
<Route path="/foo/bar" element={<FooBar />} />
</Route>
</Routes>but doesn't for a case like ours which looks more like <Routes>
<Route path="/foo/*" element={<Foo />} />
</Routes>with <Routes>
<Route path="/foo/bar" element={<FooBar />} />
</Routes>So until you come across remix-run/react-router#10321 / remix-run/react-router#9841 that explicitly point this out, you're likely to find confusing results talking about it working because they only consider the first case. |
This is PR bumps Jetpack-the-plugin to use
react-router-domto v6 (prior to migrating everything else in the monorepo to v6 (see #40705) and ultimately to v7).Proposed changes:
Essentially I followed this guide:
https://reactrouter.com/6.28.0/upgrading/v5
Of note:
<Prompt>is no longer supported in v6, so I've commented the one instance out.withRouterinstances were updated. These really should have been upgraded prior to moving to v5.1 (e.g. in Update/react router 5 #15514 or Update dependency react-router-dom to v5.2.0 #15821), but better late than never.history.listenwas updated (h/t @anomiex for a solution here); see inline comment.That last item could probably use the most scrutiny in a code review.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: