nusmods
nusmods copied to clipboard
Adopt React concurrent mode
Context
This is another item split out of #2922. This PR:
- Installs experimental versions of React and React DOM.
- Switches the whole app to use concurrent mode.
- Sets up tests to run in concurrent mode.
This PR lays the groundwork for future work involving React Suspense for data fetching (e.g. #2922). I'm adopting now as the performance issues in #3038 can be very easily solved with concurrent mode -- this is done in #3040, which is stacked on top of this.
This PR is stacked on #3038.
Concurrent Mode? Now?
Concurrent mode seems stable enough to adopt, having been in use at Facebook for a while. As React Suspense cause components to be written differently, I think it's better to adopt it early so that our components are future-proof. Best practices with React Suspense require CM, especially its useTransition
API. As NUSMods is a small app, any API changes should be easy enough to fix.
I think the biggest technical risk is bugs. CM has issues with tearing, especially with third party packages that haven't been updated for CM. If we encounter any of these, we'll likely have to patch them ourselves or find replacement packages.
Other Information
Manual testing is probably required, but I couldn't find any issues while manually poking.
Codecov Report
Merging #3039 (de2dee6) into eliang/overhaul-venues-components (381ced3) will decrease coverage by
0.03%
. The diff coverage is0.00%
.
@@ Coverage Diff @@
## eliang/overhaul-venues-components #3039 +/- ##
=====================================================================
- Coverage 57.20% 57.16% -0.04%
=====================================================================
Files 257 257
Lines 5283 5286 +3
Branches 1205 1206 +1
=====================================================================
Hits 3022 3022
- Misses 2261 2264 +3
Impacted Files | Coverage Δ | |
---|---|---|
website/src/entry/main.tsx | 0.00% <0.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 381ced3...de2dee6. Read the comment docs.
Deployment preview for de2dee65
ready at http://5fddcd4e2c063cf9fa14e0b2--nusmods-deploy-preview.netlify.app
My main concern isn't necessarily that concurrent mode is unstable, but that it might cause issues with versioning and updates. The version we pull is now a hash, which is awkward for judging the stability of. What is the release cadence on the concurrent branch? Do we know if this is matched to some version on the 17.x branch (it would be useful to know if some release is 17.0.2 + concurrent mode)?
Would be nice to set up a testing checklist or spreadsheet just to be able to split up the work a bit and make sure we have good coverage before this is released
What is the release cadence on the concurrent branch? Do we know if this is matched to some version on the 17.x branch
From what I can see, they make experimental releases every month or so, independent of stable, versioned releases.
https://www.npmjs.com/package/react
awkward for judging the stability of
It's definitely going to be hard to know what was changed in a particular update and whether any new issues were introduced. Any new bugs are also likely to be subtle and hard to repro, but this may not be a big concern for NUSMods as we'll also be unlikely to encounter them given how small our project is.
I personally am not too concerned about any bugs within React as these releases are deployed at FB in prod and our code doesn't do anything funky as far as I know, but we may want to focus on checking that our third party deps haven't broken somehow.
Would be nice to set up a testing checklist or spreadsheet just to be able to split up the work a bit and make sure we have good coverage before this is released
A manual test right?
Rebased on parent branch, no diff change
Rebased on parent branch, no diff change
React concurrent is so 2020. RSC is the in-thing now.