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

Upgrade to React 18 #15555

Merged
merged 1 commit into from Mar 29, 2023
Merged

Upgrade to React 18 #15555

merged 1 commit into from Mar 29, 2023

Conversation

sapkra
Copy link
Contributor

@sapkra sapkra commented Jan 24, 2023

What does it do?

  • updates react to 18
  • updates react-dom to 18
  • updates react-is to 18
  • updates react-test-renderer to 18
  • updates react-query to 3.39.2 (so it officially supports react18)
  • removes @testing-library/react-hooks
  • updates @testing-library/react to 13.4
  • Converts hook tests to use RTL renderHook

Why is it needed?

  • use up-to-date version of important library
  • improve rendering performance
  • add compatibility for React 18 monorepos
  • Means we can update a lot more libraries.

Related issue(s)/PR(s)

https://feedback.strapi.io/feature-requests/p/add-support-to-react-18

@strapi-cla
Copy link

strapi-cla commented Jan 24, 2023

CLA assistant check
All committers have signed the CLA.

@sapkra
Copy link
Contributor Author

sapkra commented Jan 24, 2023

I fixed almost all failing tests, but there a still a few I need help with.

@joshuaellis
Copy link
Member

Won't all the hook tests need updating because @testing-library/react-hooks does not support r18? 🤔

@joshuaellis joshuaellis added source: external Source is an external library pr: chore This PR contains chore tasks (cleanups, configs, tooling...) labels Jan 24, 2023
docs/package.json Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

❗ No coverage uploaded for pull request base (chore/react18@31223c9). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 8164d36 differs from pull request most recent head c147bc6. Consider uploading reports for the commit c147bc6 to get more accurate results

Additional details and impacted files
@@               Coverage Diff                @@
##             chore/react18   #15555   +/-   ##
================================================
  Coverage                 ?   47.55%           
================================================
  Files                    ?      447           
  Lines                    ?    15668           
  Branches                 ?     3342           
================================================
  Hits                     ?     7451           
  Misses                   ?     6818           
  Partials                 ?     1399           
Flag Coverage Δ
back 47.33% <0.00%> (?)
unit_back 47.33% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sapkra
Copy link
Contributor Author

sapkra commented Jan 24, 2023

Won't all the hook tests need updating because @testing-library/react-hooks does not support r18? 🤔

I refactored it, it was more than I thought. It would be great to get some feedback on this. 🙂

Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

As much as I would want React 18 in Strapi, I don't think it would be possible without causing problems with the community plugins. Many plugins use libraries that expect React ^17, running these with React 18 will create issues. I'm afraid we'll need to wait for Strapi v5 as this looks like a breaking change

@joshuaellis
Copy link
Member

As much as I would want React 18 in Strapi, I don't think it would be possible without causing problems with the community plugins. Many plugins use libraries that expect React ^17, running these with React 18 will create issues. I'm afraid we'll need to wait for Strapi v5 as this looks like a breaking change

I'd like to discuss more about this – react 18 is backwards compatible with react 17 assuming you're not using strict mode, which we're not? So why do you think updating to react 18 would cause these breaking changes? 🤔 If anything it would only cause potential install errors with npm without --legacy-peer-deps which i imagine is already a problem before this looking at my terminal at least.

@sapkra
Copy link
Contributor Author

sapkra commented Jan 25, 2023

We are using a react 18 resolution in our package.json for several months now and we never encountered an compatibility issue.

@joshuaellis
Copy link
Member

Hey @sapkra sorry for the radio silence on this one, some good news, as you can see i've updated this branch and sorted out a bunch of stuff in relation to updates in the main branch and i'm going to merge this to an internal branch so i can do an experimental release for outside testing to investigate potential issues that could occur with our ecosystem.

@derrickmehaffy
Copy link
Member

Yeah I do think we need to heavily test this against many community plugins before we consider merging. This could cause a metric ton of problems if we miss something.

Might also be good to run an extended beta here too for a few months and let people who are currently building plugins test against it also.

Also doesn't react 18 default to SSR instead of CSR? Have we tested building and ejecting the admin away from the koa instance?

Likewise if it's SSR we may have an increase in resource usage on the server and may impact our min/recommend RAM needs too.

@joshuaellis
Copy link
Member

joshuaellis commented Mar 23, 2023

h I do think we need to heavily test this against many community plugins before we consider merging. This could cause a metric ton of problems if we miss something.

I'll prep a document this morning for internal viewing.

Also doesn't react 18 default to SSR instead of CSR?

No it doesnt.

@joshuaellis
Copy link
Member

Vercel deployment is broken on main and will be fixed by #16229

vercel.json Outdated Show resolved Hide resolved
fix: update tests

refactor: replace @testing-library/react-hooks

chore: fix bad rebase

chore: update snapshots

fix: fe tests

chore: cleanups

chore: bump react-query to support react 18

<3

chore: re-run yarn

chore: add skip-cache to build:ts

chore: add correct command

Revert "chore: add skip-cache to build:ts"

This reverts commit e5a8c60.
@joshuaellis joshuaellis merged commit fd5e8bf into strapi:chore/react18 Mar 29, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: chore This PR contains chore tasks (cleanups, configs, tooling...) source: external Source is an external library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants