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

React 18 support in peerDeps #76

Closed
Chupriarti opened this issue Apr 7, 2022 · 9 comments · Fixed by #89
Closed

React 18 support in peerDeps #76

Chupriarti opened this issue Apr 7, 2022 · 9 comments · Fixed by #89
Labels
kind: feature New feature or request scope: dependencies Pull requests that update a dependency file scope: tests Tests could be improved. Or changes that only affect tests

Comments

@Chupriarti
Copy link

Is it possible to update peerDependencies for React 18 support?

@agilgur5
Copy link
Owner

agilgur5 commented Apr 7, 2022

Unfortunately it's not quite as simple as updating a peerDep as the devDep and testing suite has to be updated as well.
Per #64 (review) , the whole test harness basically has to be rewritten as Enzyme was never fully updated to support React 17, let alone React 18

@agilgur5 agilgur5 added the scope: dependencies Pull requests that update a dependency file label Apr 7, 2022
@Chupriarti
Copy link
Author

Thank you for your reply.
So I guess this issue can be close, until other code will be updated.

@agilgur5
Copy link
Owner

agilgur5 commented Apr 8, 2022

No, it should remain open as it is an existing and relevant issue. In particular, it would be good to see how many upvotes it garners compared to other issues/features/etc in order to help measure priority

@agilgur5 agilgur5 reopened this Apr 8, 2022
@agilgur5 agilgur5 changed the title React 18 support React 18 support in peerDeps Apr 12, 2022
@Rick83600

This comment was marked as spam.

@LJGrant

This comment was marked as spam.

@agilgur5
Copy link
Owner

agilgur5 commented May 13, 2022

Any chance that is is being worked on?

There is a label that indicates whether something is being worked on.
Otherwise, please see this blog post written by popular OSS author sindresorhus and inspired by ESLint author nzakas.

If so, any eta on when this might be resolved?

If you (and your company) need this quickly, you are more than welcome to contribute. I am a volunteer working on this in my limited free time and on top of that, am spread pretty thin amongst several OSS projects.

As I've outlined above already, please see #64 (review) for what that entails:

  1. rewriting the test harness with RTL and react-dom-instance
  2. making sure all tests still pass and that such a refactor is actually compatible (part of the complexity is the unknown)
  3. then upgrading to React 18 in peer and devDeps
  4. and finally ensuring the tests pass and fixing anything that may have broken in React 18 (another unknown)
  5. (optional) adding a test for concurrent mode, while it is off by default, would also help with forward compatibility.

I believe the example and the "Usage" section of the docs need updating to createRoot etc.
Not sure if anything else will break; off the top of my head if React 17's JSX is required in React 18, this actually might require a breaking change (since it is currently compatible down to React 14)

@agilgur5
Copy link
Owner

agilgur5 commented Jun 15, 2022

rewrote it myself

Unfortunately, no one stepped up to rewrite the test harness despite this issue's significant popularity (see the upvotes, 16 at time of writing), which is not a particularly good bellwether for open-source stewardship, especially when Enzyme (a much bigger library) hasn't had many folks step up either.

I eventually found time for this, but I'm not exactly a big user of this library or have a particular need for it, given that the company I wrote it for hasn't existed in years.

react-dom-instance only works with React 16; fortunately, was able to workaround that

  1. rewriting the test harness with RTL and react-dom-instance
  2. making sure all tests still pass and that such a refactor is actually compatible (part of the complexity is the unknown)

That complexity got hit pretty hard as I found that react-dom-instance doesn't even work with React 17 (or React 18 for that matter), see my issue arqex/react-dom-instance#14 that contains a tiny repro using react-signature-canvas.

Fortunately, I was able to workaround that by using React refs in the tests themselves, see my PR: #88
That PR also goes over some of the non-trivial differences between Enzyme and RTL; RTL is very much designed as an integration testing library, which is not particularly well-suited for React component libraries like this one, where there isn't necessarily a whole UI to test against like there may be with React apps.

React 18 support should be out in a bit

  1. then upgrading to React 18 in peer and devDeps
  2. and finally ensuring the tests pass and fixing anything that may have broken in React 18 (another unknown)

I tested #88 with React 18 as well and it was working without much issue fortunately. I'm still going to do some more testing to make sure of that, but will probably release React 18 support in peerDeps in a bit.

Will also backport this to v1.0.x as the TS rewrite (#42) has not been released in v1.1.0 yet and definitely deserves a separate release from this.

@agilgur5
Copy link
Owner

5. (optional) adding a test for concurrent mode, while it is off by default, would also help with forward compatibility.

I believe the example and the "Usage" section of the docs need updating to createRoot etc. Not sure if anything else will break; off the top of my head if React 17's JSX is required in React 18, this actually might require a breaking change (since it is currently compatible down to React 14)

Using createRoot doesn't seem to be required but does work with this component. And React 17's new JSX is not required in React 18, so seems to be all good on that front ✅

Will also backport this to v1.0.x

Did have some difficulties backporting the example as react-hot-loader doesn't support React 18 either (gaearon/react-hot-loader#1808). Although the example still works fine 🤷 .
Tests were having some issues due to node-canvas (similar to #81 (comment)) as well, but things work externally otherwise.

Will just definitely have to release the TS rewrite in v1.1.0 soon in order to not deal with various old dependency issues anymore.

@agilgur5
Copy link
Owner

Released in v1.0.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New feature or request scope: dependencies Pull requests that update a dependency file scope: tests Tests could be improved. Or changes that only affect tests
Projects
None yet
4 participants