Final steps in the react-router v6 migration! 馃帀
#4377
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ivarnakken
added
types
ivarnakken
changed the title
react router v6 migration! 馃帀react-router v6 migration! 馃帀
ivarnakken
force-pushed
the
further-steps-in-react-router-v6-migration
branch
from
67884c2
to
643802d
Compare
ivarnakken
force-pushed
the
finish-react-router-migration
branch
from
82835c3
to
426f51f
Compare
ivarnakken
force-pushed
the
further-steps-in-react-router-v6-migration
branch
from
643802d
to
cddcd9d
Compare
ivarnakken
force-pushed
the
finish-react-router-migration
branch
from
426f51f
to
6eef4c7
Compare
It is not compatible with `react-router` v6.4, and is not something we "really" need.
ivarnakken
force-pushed
the
finish-react-router-migration
branch
from
6eef4c7
to
9b30c54
Compare
|
Great job 馃コ I'll try to review your code, although that's quite a daunting task 馃槄 I'll, at the very least, run the code and test if everything works alright :)) |
All uses of `useHistory` had to be replaced with `useNavigate`.
`react-router` v6 does not allow context to be passed as a prop to `StaticRouter`, so a simple workaround was to just create our own React Context.
Also remove a bunch of stub @types packages. Some are already provided in the packages themselves, and the `react-router` specific ones caused some compatability issues for some reason. And fix a small ts error in the ConfirmModal code preventing build to complete.
ivarnakken
force-pushed
the
finish-react-router-migration
branch
from
9b30c54
to
7028a69
Compare
|
This migration actually removes 305 ts errors! |
I've noticed several issues with running `react-router` v6 on `node` v16. It was about time anyways .. For some reason I now had to update some stylelint stuff?
The tests relied on passing props into the page components, but this is no longer accepted. Nevertheless, what they try to test is already tested in our e2e tests, and besides it's quite uncommon to unit test entire pages.
ivarnakken
force-pushed
the
finish-react-router-migration
branch
from
deaf593
to
9d73130
Compare
norbye
approved these changes
Jan 9, 2024
ivarnakken
force-pushed
the
finish-react-router-migration
branch
2 times, most recently
from
3b183b1
to
314a592
Compare
|
We're finally passing CI! I never thought this day would come when I started this migration back in August hehe |
|
@haukkagu @norbye Made some few additions that you may want to look over 馃槃 |
* Issues with a default pool not appearing when picking an appropriate event status type * Pool merge time was being validated when it was not supposed to * useMazemap would always be true, even when a normal location had been set * Not actually in the editor, but it was previously not possible to view registrations on the event detail page because of the wrong check for `hasFullAccess` * Better (and accurate) validation
Among other things, `SubmitButton` now accepts a `disabled` prop in case you want to check for more things.
When the user had no prior meetings, `meetingSections.length` would always be `0`, leading to an endless loop of fetching. Fixed by introducing a simple flag.
Not sure why `pagination` was added to the dependency array, but nevertheless this caused an endless loop of fetching. The "fetch more" button seemed to not work without some proper feedback, so I added that as well.
The query should be in the dependency array to force a fetch when switching the page (the query).
ivarnakken
force-pushed
the
finish-react-router-migration
branch
from
45162bb
to
db7c58a
Compare
Extend the NavLink wrapper to accept a list of paths to "accept".
I believe the core of the issue was the spread operator for the query in the fetch action, but I'm too tired to look into exactly why that was.
ivarnakken
force-pushed
the
finish-react-router-migration
branch
from
db7c58a
to
3c9505d
Compare
All fields were disabled when creating a new restricted mail because the `isNew` check was wrong. Unlike for the forms for email lists and users, the url param here will not be `new` but rather `undefined`.
`endpoint` would be `undefined`, and a bad API request would be made.
Base automatically changed from
further-steps-in-react-router-v6-migration
to
migrate-away-from-connected-router
January 11, 2024 11:42
ivarnakken
merged commit 7656be4
into
migrate-away-from-connected-router
4 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Remove redux-first-history
It is not compatible with
react-routerv6.4, and is not something we"really" need.
Remove compatabillity package and bump react-router to v6
All uses of
useHistoryhad to be replaced withuseNavigate.Fix ssr routing with context
react-routerv6 does not allow context to be passed as a prop toStaticRouter, so a simple workaround was to just create our own ReactContext.
Make lego-bricks compatible with webapp's upgraded router
Also remove a bunch of stub https://github.com/types packages. Some are already provided in
the packages themselves, and the
react-routerspecific ones caused somecompatabillity issues for some reason.
And fix a small ts error in the ConfirmModal code preventing build to complete.
Testing
SSR seams to work completely fine. Tried to visit most routes, no problems.
However, I did notice some problems with building on node 16, so we ought to bump that very soon.