-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
UI: Migrate router to react-router #16440
Conversation
# Conflicts: # lib/api/package.json # lib/router/package.json # yarn.lock
Nx Cloud ReportCI ran the following commands for commit c325477. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch
Sent with 💌 from NxCloud. |
I have some more work todo on making all stories pass that wanted the router with a mocked value |
# Conflicts: # lib/api/package.json # lib/router/package.json # yarn.lock
… required in react-router
hmm, the preview stories seem to still be missing some data from the manager state. The app stories look like correct changes to me. |
Tried it out in the example projects and it's looking pretty great @ndelangen! If we can get those stories fixed, do you think it's safe to merge for 6.4? |
I find it hard to say, how safe it is to merge into 6.4. I'm 95% confident it should be fine tbh. There could be complications if a user is using react-router and is mocking it or something. Then it might break in docs, maybe. Again it's hard to say. I suspect the conflicts will be minimal, but if they happen the user can reach out to me, and if I get a repro I can debug. |
Thanks so much @ndelangen! When those stories are fixed, I'm happy to try on our private repo as a next release. |
To test:
|
"core-js": "^3.8.2", | ||
"fast-deep-equal": "^3.1.3", | ||
"global": "^4.4.0", | ||
"lodash": "^4.17.20", | ||
"memoizerific": "^1.11.3", | ||
"qs": "^6.10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests are still using it, so it should stay in devDependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you can make a PR? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: #14619
What I did
I migrated storybook to use react-router v6 (seems to be in beta)
react-router: https://github.com/remix-run/react-router
I following this migration-guide:
https://github.com/remix-run/react-router/blob/main/docs/guides/migrating-reach-to-6.md
I took the minimal resistance path, meaning I have retained the
Location
component and how it works.Even though this is NOT something react-router provides.
Instead react-router provides hooks and Route components, but retrieving the location has to be done using a hook.
I agree that this is a good change, and we should do it at some point, but baby-steps.
Let's provide the upgrade works, and then take further steps.
How to test
If your answer is yes to any of these, please make sure to include it in your PR.