React Router upgrade: main app (part 1) #771
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #771 +/- ##
==========================================
+ Coverage 81.92% 82.13% +0.20%
==========================================
Files 238 240 +2
Lines 4853 4882 +29
Branches 1310 1321 +11
==========================================
+ Hits 3976 4010 +34
+ Misses 841 836 -5
Partials 36 36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/hooks/useQuery.js
Outdated
There was a problem hiding this comment.
We might as well replace useQuery with useSearchParams right now.
Or, to keep this PR clean, we can do it in a separate PR 🙂
There was a problem hiding this comment.
yeah that's the plan - do this afterwards!
…nning of a form * Navigate from the start to introduction page and first step * Navigate from start directly to first step when no introduction page is used.
In v5, this hook probably didn't exist yet and that's why we added it, but v6 has a convenience hook already. Our own implementation now simply wraps it, and we can deprecate it at some point and use react router's hook instead, as it also comes with a setter.
…nce implementation into hook Rather than sprinkling the param processing through various parts of the code base and repeat the implementation, provide a custom hook that encapsulates the implementation details. The hook reads the reference from the query params, and provides a helper to mutate the reference in (derived) react houter URLs. Note that the window.location.origin as base is just a workaround to be able to use the `URL` constructor and object for query string manipulation. The actual real window.location is irrelevant, as that state is managed by react router. We only care for the path (or rather the path segment) that may have some query params attached to it.
… into separate component Created a separate component that handles the redirect logic from the landing page, using the context rather than props to get the necessary information. This makes it possible to use the component as element in a statically defined route.
…tatic route definitions Created a static route definitions file for routes nested under the Form component. They are rendered in the specified outlet node, and included in the main app routes definition. The rest of the <Route> elements will follow in separate commits.
…nent to require no props Instead of passing the extra parameters, we extract the initial data reference using the hook from the LandingPage component refactor.
Update the tests to prepare for refactors in the implementation. * Wrap the component in a proper data router instead of the JSX/component router. * Replace the useQuery mocks with actual locations/url information that contains the parameters so we can use the real react-router behaviour and don't need the mocks in the first place, which reduces tight coupling.
…m query instead of prop The FormStart component now grabs the initial data reference directly from the URL parameters rather than requiring the parent component needing to do this and pass it down as a prop. This is a first step towards not needing to specify props at all to FormStart so that it can be included in the data router routes (statically).
…ead of prop in FormStart We wrap the app in form context anyway when the SDK is initialized, as the form is statically loaded only once initially when the SDK bootstraps. This means we do not need to pass it as a prop, nor do we need to worry about too-frequent re-renders.
…ur mock functions The mock functions should at least be easier to centrally update, while we still have the ability to override/specify the properties that are relevant to the test/behaviour being tested.
To allow debugging/commenting out code, ensure the linter doesn't error and stop you in your tracks, but instead log the warnings to console. The production build and CI pipeline still enforce the strict rules.
The app debug state is now displayed on the session-expired page/route, which can give some more insight on what happens with the session reset etc. Additionally, when the expiry date is reset it's set to null, and the delta calculations are off/weird because of Javascript doing math on 'null' objects. Now, we display a dash to indicate there's no session expiry at all. This was discovered during testing if certain code is still required or not.
…t into own route The component is refactored with the goal of configuring it in a static route definitions file, which means it cannot take any dynamic props as input as that is not statically available. The most important props are the current submission in the session, initial data reference and callbacks for when a submission is created or session stopped (because the user logs out, for example). For the initial data reference, we can use the hook from earlier refactors to grab it directly from the query parameters. The submission and relevant callbacks are now provided by the container/wrapper Form component that manages the submission state, through context. We have to be careful here to properly memoize callbacks as there's quite a substantial risk of running into infinite re-renders locking up the browser. Hopefully in the future we can remove those footguns by using loaders and actions of the react-router library. The core onFormStart callback is hereby also moved from the Form component to the FormStart component which properly localizes the related behaviours. We only send the created submission back up to the parent state when the creation is done. Additionally, I've opted to use 'named arguments' for the anonymous flag to make the code easier to read/understand. Secondly, the browser event doesn't need to be passed to the callback, since the form submit event is already suppressed when the callback is invoked. Finally - the previous version of this code would also reset the session expiry whenever a submission is started. This was required to get rid of the session expired error message. This had become obsolete when the session expiry was moved to its own route and component, which already resets the session automatically. No expiry messages are displayed when you navigate away from the expiry notice to the start page by clicking the restart link.
… tests Made the tests more robust by doing proper URL parsing instead of string comparisons, as the order of query string parameters is not important or deterministic. The router wrapper is updated so we can pass the current location without monkeypatching the global window object, which also cleans up the test a bit more. Finally, the onFormStart callback is updated since we don't pass an event any longer - it is handled on the form submit action already.
Updated the existing tests for the new component structure and injection of dependencies, and some tests were moved from Form to FormStart tests because the behaviour being tested was also moved. Less mocks are now used and the tests should generally be a bit more robust.
sergei-maertens
force-pushed
the
issue/4929-react-router-upgrade-main-app
branch
from
c38fe5b to
cde10d2
Compare
Bundle ReportChanges will increase total bundle size by 661 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
| @@ -9,6 +9,7 @@ import { | |||
| appointmentRoutes, | |||
| manageAppointmentRoutes, | |||
| } from 'components/appointments'; | |||
| import formRoutes from 'components/formRoutes'; | |||
There was a problem hiding this comment.
once we're around to this refactoring, I want to move the route definition files too, since this is not a component :)
| </ErrorBoundary> | ||
| } | ||
| /> | ||
|
|
||
| <Route |
There was a problem hiding this comment.
rest will be done in a different PR, this one is already quite big
| @@ -51,69 +49,6 @@ const Wrapper = ({form = buildForm(), initialEntry = '/startpagina'}) => { | |||
| ); | |||
| }; | |||
|
|
|||
| test('Start form anonymously', async () => { | |||
There was a problem hiding this comment.
tests are moved/merged into the FormStart tests
The initial data reference is no longer provided through props, but taken from the (router) context so we must set up the story accordingly.
| const FormLandingPage = () => { | ||
| const {introductionPageContent = ''} = useFormContext(); | ||
| const {addInitialDataReference} = useInitialDataReference(); | ||
| const startPageUrl = introductionPageContent ? 'introductie' : 'startpagina'; |
There was a problem hiding this comment.
Im wondering if we can replace the route strings with constants/enums/something.
If you agree, this is probably better suited as a stand-alone improvement :)
There was a problem hiding this comment.
I'm not sure if it's worth the indirection - it's one of the things I miss in react-router that django has: naming routes and reversing the URLs.
I also seem to remember that other routing libraries have support for strongly typed routes where typescript would catch errors, however I think react-router doesn't support it and we're not using TS yet.
Still - I think this kind of dynamic behaviour requires a test that paints the correct behaviour in different cases and it must do so by asserting what's visible on the target route page.
I'm also not sure how practical this will be when you need to add dynamic parts of a route - you end up with constants in literal strings with interpolation... not sure how that affects readability.
| onDestroySession: () => {}, | ||
| }); | ||
|
|
||
| const SubmissionProvider = ({ |
There was a problem hiding this comment.
I think it would be nicer to move this to a separate file.
Maybe a src/context/SubmissionContext, which exports the SubmissionProvider and the useSubmissionContext
There was a problem hiding this comment.
I disagree because I'd prefer keeping the code together where it's used instead of smearing out stuff that is used together across different files. When all this is done, the Form component should be a small component with only minimal callbacks defined at this layer - the rest should be pushed down to individual routes.
With these refactors I'm still iterating a lot and the context implementation details will change - IMO it's easier to do this while they're close together.
Depends on #769
Partially fixes open-formulieren/open-forms#4929
Extracted some of the routes from the
Formcomponent and refactored some implementation details.I've run the backend e2e tests against a (production) build of this branch and they all pass.