Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt React concurrent mode #3039

Closed

Conversation

taneliang
Copy link
Member

@taneliang taneliang commented Dec 8, 2020

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
Copy link

codecov bot commented Dec 8, 2020

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.

@nusmods-deploy-bot
Copy link

nusmods-deploy-bot bot commented Dec 8, 2020

@taneliang taneliang marked this pull request as ready for review December 8, 2020 15:11
@taneliang taneliang requested a review from a team December 8, 2020 15:11
@ZhangYiJiang
Copy link
Member

ZhangYiJiang commented Dec 13, 2020

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
Copy link
Member

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

@taneliang
Copy link
Member Author

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
Copy link
Member Author

Rebased on parent branch, no diff change

Copy link
Member

@li-kai li-kai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ⚛️

@taneliang taneliang force-pushed the eliang/concurrent-mode branch from 156c1e3 to de2dee6 Compare December 19, 2020 09:48
@taneliang
Copy link
Member Author

Rebased on parent branch, no diff change

@li-kai
Copy link
Member

li-kai commented Mar 27, 2024

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

@li-kai li-kai closed this Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants