React router upgrade part 3 #776
Open
+979
−1,345
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
sergei-maertens
force-pushed
the
issue/4929-react-router-upgrade-part-3
branch
from
dff444a to
0cfefdb
Compare
Bundle ReportChanges will decrease total bundle size by 2.83kB (-0.06%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
sergei-maertens
force-pushed
the
issue/4929-react-router-upgrade-part-3
branch
from
0cfefdb to
16de5e6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #776 +/- ##
==========================================
+ Coverage 82.19% 83.17% +0.97%
==========================================
Files 241 238 -3
Lines 4887 4736 -151
Branches 1319 1276 -43
==========================================
- Hits 4017 3939 -78
+ Misses 835 762 -73
Partials 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
It became obsolete by switching to the react-vite builder.
…work layer Instead of mocking the hooks, mock the API layer for more accurate behaviour and less tight coupling with implementation details.
…issionSummary component Rather than dynamically providing the props from the parent component, ensure that the parent(s) set up the context properly and take the necessary information from there so that we can statically declare the route for the submission summary.
Upgraded the packages in an attempt to debug why a new test would keep running forever without crashing/erroring out, seemingly being stuck in an infinite loop. I suspected something odd with timers/setTimeout going on and there are some open issues in testing-library/vitest related to that, but this was not the problem. I've decided to keep the upgrades since things *appear* to work with the new versions, but of course only CI will decide that for real. If we can keep this 'free' upgrade, that's a nice accident, if it's problematic, I revert it.
…ScrollIntoView jest-dom doesn't provide the scrollIntoView method on HTML elements for it's JS API, which explains why this code/flow appears to work properly when using a real browser and simulating testing flows manually, while being problematic in tests. The problem here was ultimately that no mock was set up for this method, causing the useEffect hook to crash because undefined is not a function that can be called. This problem is then compounded because React will render the nearest ErrorBoundary, which we have define in most of our routes (that are currently under testing), and these error boundaries use the ErrorMessage component too, which also calls useScrollIntoView, which also crashes and seemingly gets stuck in an infinite render, causing Vitest to keep running forever without erroring or even timing out (presumably because the JS thread is just stuck in an infinite loop). The fool-proof way to mitigate this is to nullish check the function before calling it, so that future oversights (forgetting to mock) will not cause us to spend hours of debugging again. For the tests, the scrolling behaviour is not relevant anyway. A test is added to establish the assumption that this callback doesn't exist, which protects against accidentally setting up a mock in this test file.
…sion completion This endpoint was not mocked yet, apparently. We can now properly write tests that simulate the submission/completion of a form and the behaviour that happens afterwards.
…UrlPoller component * Use nullish-check for callback invocation which is a bit more idiomatic Javascript * Throw error instead of just logging it so that it bubbles to the nearest error boundary. This makes the flow of rendering easier to follow/debug/understand.
* Take out the component-invariant function and move it to the module level. This gives it a stable reference as it doesn't depend on any components state or props. * Throw errors instead of just logging them for a more obvious error handling flow.
…> error submission flow There's now a test to lock in the behaviour after confirming the submission, getting the status page with spinner and redirect back to summary page if there's a background processing error. Now we can refactor the error message state handling to get these props out of the Summary component. The fake timer situation is... complicated. See testing-library/react-testing-library#1197
…r state Instead of tracking this state at the component level, we can pass it while redirecting back to the overview page, which removes the need for some props. Navigating to another location clears the error state again and it is updated with new submit attempts.
… management Removed reducer that only supported one action type, in favour of a plain useState hook.
sergei-maertens
force-pushed
the
issue/4929-react-router-upgrade-part-3
branch
from
16de5e6 to
e0484cf
Compare
…sUrl prop The Form/SubmissionSummary components no longer need to manage the processing status URL (backend API URL) state to be passed down to other components, instead they're now passed via the router location state. The payment start and confirmation page now grab them from the location state. Appointments uses the query string parameter for the status URL, but this should at some point all be moved to session storage so that we can survive hard refreshes (which may happen by accident or by impatient users), as it is more secure since sharing the status URL is enough to get access to the submission data via the PDF.
sergei-maertens
force-pushed
the
issue/4929-react-router-upgrade-part-3
branch
from
d155158 to
c750dcc
Compare
…m prop By passing the submitted submission in the router state, we don't need to track it in the parent component any longer to be able to restore it when there are failures.
… static route declarations
sergei-maertens
changed the title
…isn't broken because of previous refactor The Form component sometimes needs to grab the submission from the router state too, in particular to render the payment start page.
sergei-maertens
force-pushed
the
issue/4929-react-router-upgrade-part-3
branch
from
917003d to
9697156
Compare
Dispatcher actions were obsoleted by earlier refactor work.
…etion state We can infer the submission _complete call/result state from the presence of the status URL in the router state, so we no longer need to pass an onConfirmed callback down to set this in the parent state.
…irmation status view page
sergei-maertens
force-pushed
the
issue/4929-react-router-upgrade-part-3
branch
from
bd275da to
8bb788d
Compare
…t processing error We can grab the error from the state now and make it disappear when a new submit attempt is made with local component state rather than needing to use or update the context.
It's no longer necessary.
The error handling of the post method is obsoleted because the apiCall helper itself already calls throwForStatus which results in an exception if/when response.ok is false, so this block of code will never execute. This also further simplifies the helper to directly return the status URL, which is the only bit of relevant information returned by the API endpoint.
…into StatusUrlPoller The StatusUrlPoller component is tightly coupled with the particular submission status API endpoint anyway - it already checks specific props to determine whether it's failing or succeeding. Since the failure handlers are the same in both places, we can localize this behaviour/the details in the component itself and only pass the particular route it should redirect/navigate the user to which is responsible for display the error message. This simplifies quite a bit of code. Maybe at some point we can also get rid of the success callback used by the appointments flow.
Replace the reducer with a simple useState - the complex state management was obsoleted by the previous refactors.
…ing into own component Instead of hooking this up to the Form component, we can instead wrap the context provider and let it fetch the config by itself. This way, we slim down the Form component, and get a small performance boost since loading the analytics tool config is not immediately relevant. Since context is used, only when the config loading is resolved will the consumers be re-rendered, which at this point is mostly the AbortButton component. We deliberately don't show a loader when the config is being retrieved as this would block the entire page without good reason and we have a faster time-to-interact this way.
… own component The component was moved from Form.jsx into its own file and imports are updated.
RequireSubmission now always takes the submission from the context and requires children to be specified instead of a render prop/component.
…ulations from Form component Now that we have all the necessary information available via context, we can encapsulate all the progress indicator render logic in a single component instead of polluting the 'container' Form component with it.
sergei-maertens
force-pushed
the
issue/4929-react-router-upgrade-part-3
branch
from
65a2e83 to
963ead2
Compare
Mocks, test helpers etc. shouldn't bring down code coverage - they're helper tools.
…a central location This should make it easier to map out the URL/route structure and figure out what gets rendered where and when.
We now have a single entry point for the SDK routes that can easily be re-used in tests, storybook and by the actual application. If needed, specific submodule routes can be loaded with their appropriate import paths. One advantage of this is that component files can follow the best practice of only exporting the component as default without also exporting the routes, and the route definitions are no longer in the 'components' folder, which is more semantically correct.
robinmolen
reviewed
Jan 20, 2025
| } | ||
| // spinners. Throwing during rendering will at least make it bubble up to the nearest | ||
| // error boundary. | ||
| if (error) throw error; |
There was a problem hiding this comment.
This throw is not needed, right? Because the error already gets thrown by line 50
There was a problem hiding this comment.
indeed - something didn't go right here in the refactor, good catch!
robinmolen
reviewed
Jan 20, 2025
There was a problem hiding this comment.
The if (result === RESULT_FAILED) on lines R91-R93 can now be removed, right?
Seeing how onFailureNavigateTo is required, and unhandled result === RESULT_FAILED should no-longer be possible
At the very least, the error message should be changed to Failure should have been handled with the onFailureNavigateTo prop.
There was a problem hiding this comment.
nice catch - since the prop is required and the failure handling is localized, this is indeed obsolete now
robinmolen
reviewed
Jan 20, 2025
| if (state.error) errors.push(state.error); | ||
| return errors; | ||
| }; | ||
| const errorMessages = [location.state?.errorMessage, submitError].filter(Boolean); |
There was a problem hiding this comment.
Ah nice, i didn't know you could do .filter(Boolean) 👌
robinmolen
reviewed
Jan 21, 2025
| vi.useRealTimers(); | ||
| await waitForElementToBeRemoved(loader); | ||
|
|
||
| // due to the error we get redirected back to the summary page. |
There was a problem hiding this comment.
I think this comment is incorrect?
robinmolen
reviewed
Jan 21, 2025
| ); | ||
| export const mockSubmissionPaymentStartPost = ( | ||
| data = {type: 'get', url: 'https://example.com', data: {}} | ||
| ) => http.post(`${BASE_URL}payment/:uuid/demo/start`, () => HttpResponse.json(data)); |
There was a problem hiding this comment.
Is there a reason to change method to type? If i understand it correctly, type is used for something else https://developer.mozilla.org/en-US/docs/Web/API/Response/type, https://github.com/mswjs/msw/blob/e3234fdce5a36f41c77719492e2eca0f77d7aaa6/src/core/HttpResponse.ts#L9.
If this doesn't clash, its fine. But this could result into different behavior in the tests vs real http.post actions.
data should probably also be changed to body
robinmolen
reviewed
Jan 21, 2025
| import {buildGovMetricUrl} from 'components/analytics/utils'; | ||
| import useFormContext from 'hooks/useFormContext'; | ||
|
|
||
| const AbortButton = ({isAuthenticated, onDestroySession}) => { | ||
| const intl = useIntl(); | ||
| const analyticsToolsConfig = useContext(AnalyticsToolsConfigContext); | ||
| const analyticsToolsConfig = useAnalyticsToolsConfig; |
There was a problem hiding this comment.
Shouldn't this be:
Suggested change
| const analyticsToolsConfig = useAnalyticsToolsConfig; | |
| const analyticsToolsConfig = useAnalyticsToolsConfig(); |
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.
Partly closes open-formulieren/open-forms#4929
This moves the remainder of the components to the statically defined routes.