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

feat: upgrade react-router-dom #968

Closed
wants to merge 17 commits into from
Closed

Conversation

0xdef1cafe
Copy link
Collaborator

taking over original PR from #695 to manage merge conflicts and get deployed internally

Description

Upgrade React Router

close #675

Ref

Notice

Before submitting a pull request, please make sure you have answered the following:

  • Have you followed the guidelines in our Contributing guide?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • [] Do all new and existing tests pass? Does the linter pass?

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

If applicable, please link to the github issue and put closes #XXXX in your comment to auto-close the issue that your PR fixes.

Testing

Please outline all testing steps

  1. Pull branch locally and run yarn to install new deps
  2. etc...

Screenshots (if applicable)

@0xdef1cafe 0xdef1cafe requested a review from a team as a code owner February 8, 2022 19:06
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Feb 8, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 97eb23a
Status: ✅  Deploy successful!
Preview URL: https://4c768492.web-29e.pages.dev

View logs

@0xdef1cafe
Copy link
Collaborator Author

@cjthompson this compiles and builds but shits the bed at runtime - see the cloudflare link. could you take another look please?

@cjthompson
Copy link
Contributor

We cannot do this upgrade. react-router-dom v6 removes the ability to use nested routers. We use multiple <MemoryRouter>s within the default <BrowserRouter>.

We will need to revamp completely how we do routing. This also means that we may have to rethink how we do routing for plugins. We may have to have plugins export all of their routes to work with v6 (or choose another router module?).

@cjthompson cjthompson closed this Feb 9, 2022
@gomesalexandre
Copy link
Contributor

We cannot do this upgrade. react-router-dom v6 removes the ability to use nested routers. We use multiple <MemoryRouter>s within the default <BrowserRouter>.

We will need to revamp completely how we do routing. This also means that we may have to rethink how we do routing for plugins. We may have to have plugins export all of their routes to work with v6 (or choose another router module?).

What's the rationale to using MemoryRouter as a nested router? If it is to have "independent" (or rather reusable) subrouters, isn't it possible to use e.g The ${path}/relative syntax where path would be either somePath (nested route) or '' (independent route)?. MemoryRouters should be for contexts that don't have access to the browser (history) API, typically Node tests.

I think by keeping the nested routers nested, but not making them memory routers anymore, we would be able to upgrade. Not sure about the plugin implications here.

@cjthompson
Copy link
Contributor

We cannot do this upgrade. react-router-dom v6 removes the ability to use nested routers. We use multiple <MemoryRouter>s within the default <BrowserRouter>.
We will need to revamp completely how we do routing. This also means that we may have to rethink how we do routing for plugins. We may have to have plugins export all of their routes to work with v6 (or choose another router module?).

What's the rationale to using MemoryRouter as a nested router? If it is to have "independent" (or rather reusable) subrouters, isn't it possible to use e.g The ${path}/relative syntax where path would be either somePath (nested route) or '' (independent route)?. MemoryRouters should be for contexts that don't have access to the browser (history) API, typically Node tests.

I think by keeping the nested routers nested, but not making them memory routers anymore, we would be able to upgrade. Not sure about the plugin implications here.

My point is only that the way we are doing routing will require more work to change before we can upgrade and those changes were not known when the ticket was written and were not in scope of the ticket.

@gomesalexandre
Copy link
Contributor

My point is only that the way we are doing routing will require more work to change before we can upgrade and those changes were not known when the ticket was written and were not in scope of the ticket.

Oh, precisely. There is no way to implement this at 0 cost by just bumping react-router and adapting to the breaking changes, that's right!

kaladinlight pushed a commit that referenced this pull request Apr 7, 2023
* feat: all osmo txs as part of the osmo swapper need to have 0 fee so that we can swap into osmo

* no fee

* fix

* fix

* put back 0 fee
kaladinlight pushed a commit that referenced this pull request Apr 7, 2023
@0xean 0xean deleted the react-router-dom-upgrade branch October 9, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upgrade react-router-dom
4 participants