search

agilgur5 / react-signature-canvas

A React wrapper component around signature_pad (in < 150 LoC). Unopinionated and heavily updated fork of react-signature-pad
https://agilgur5.github.io/react-signature-canvas/
Other
546 stars 119 forks source link

React 18 support in peerDeps #76

Closed Chupriarti closed 2 years ago

Chupriarti commented 2 years ago

Is it possible to update peerDependencies for React 18 support?

agilgur5 commented 2 years ago

Unfortunately it's not quite as simple as updating a peerDep as the devDep and testing suite has to be updated as well. Per https://github.com/agilgur5/react-signature-canvas/pull/64#pullrequestreview-876834510 , the whole test harness basically has to be rewritten as Enzyme was never fully updated to support React 17, let alone React 18

Chupriarti commented 2 years ago

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

agilgur5 commented 2 years ago

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

Rick83600 commented 2 years ago

Need the update of peerDependencies React 18 please

LJGrant commented 2 years ago

Any chance that is is being worked on? If so, any eta on when this might be resolved?

agilgur5 commented 2 years ago

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 https://github.com/agilgur5/react-signature-canvas/pull/64#pullrequestreview-876834510 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 commented 2 years ago

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 https://github.com/arqex/react-dom-instance/issues/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 commented 2 years ago

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 (https://github.com/gaearon/react-hot-loader/issues/1808). Although the example still works fine 🤷 . Tests were having some issues due to node-canvas (similar to https://github.com/agilgur5/react-signature-canvas/issues/81#issuecomment-1146626633) 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 commented 2 years ago

Released in v1.0.6