nusmods icon indicating copy to clipboard operation
nusmods copied to clipboard

Adopt React concurrent mode

Open taneliang opened this issue 4 years ago • 7 comments

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.

taneliang avatar Dec 08 '20 13:12 taneliang

Codecov Report

Merging #3039 (de2dee6) into eliang/overhaul-venues-components (381ced3) will decrease coverage by 0.03%. The diff coverage is 0.00%.

Impacted file tree graph

@@                          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.

codecov[bot] avatar Dec 08 '20 13:12 codecov[bot]

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)?

ZhangYiJiang avatar Dec 13 '20 03:12 ZhangYiJiang

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

ZhangYiJiang avatar Dec 13 '20 03:12 ZhangYiJiang

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

image

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?

taneliang avatar Dec 13 '20 04:12 taneliang

Rebased on parent branch, no diff change

taneliang avatar Dec 18 '20 08:12 taneliang

Rebased on parent branch, no diff change

taneliang avatar Dec 19 '20 09:12 taneliang

React concurrent is so 2020. RSC is the in-thing now.

li-kai avatar Mar 27 '24 07:03 li-kai