-
-
Notifications
You must be signed in to change notification settings - Fork 116
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
add React 17 to peerDep range -- needs to update devDeps and tests #64
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
have to re-set-up Travis CINormally I could merge in something like this pretty quickly, but apparently CI has been down for like 6+ months as Travis decommissioned their Will have to re-set-up that and make sure all tests pass before merging thanks for the detail in the PR!
Thanks for logging the error here and noting they are just warnings @atomicpages ! I haven't done much front-end dev in the past year (my day job tends to be SRE-focused past few years) so I haven't actually tried NPM 7 myself yet. maintainers are human too, and also volunteersTo the other commenters, friendly reminder that one-word comments are spammy and you can use reactions instead. Also if a person has set on their status "Taking a break from toxicity and abuse 😞", probably not appropriate to spam their handle repeatedly, especially when GitHub tells you their status when doing so. 😐 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately there are some issues with this PR.
Per #58 (comment), the devDeps need updating as well so that the tests use React 17.
This is actually a good bit easier said than done because the Enzyme Adapter has to be updated. Per enzymejs/enzyme#2429, no official adapter is available yet for React 17.
React 17 has been a complicated change for Enzyme, and as it only has 1 active contributor, such complex roadblocks have caused it to stall.
There is an unofficial adapter with partial support for React 17 though. It is likely enough for the use-case within this repo, so I'll try to get in an upgrade with that.
That being said, the author of the unofficial adapter has stated that React 18 support requires even more complicated changes. They suggest using RTL, but it actually doesn't support instance methods, which, per my first testing issue, is a blocker as react-signature-canvas
actually has a ref-based API and so needs tests for it.
So while we may be able to get by unofficially with the unofficial React 17 adapter, there's not much of a path for testing React 18 as of now.
Per this blog post, looks like |
Replaced by #69 |
Fixes npm 7 install errors (really warnings)