Support React 18 as a peer dependency #312
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Based on #293, it appears this package works fine with React 18
This resolves a whole bunch of warnings and build issues from using out of date packages. Became sort of necessary when we couldn't npm install anymore because our out-of-date node-sass version demanded Python v2.
We've expanded our peer dependencies to allow React 18. This isn't really a breaking change, but usage of this with React >16 is under-tested. So we're going to do a major version bump so that any projects that depend on this package don't implicitly pick up this new version, resolve to a different version of React, and suddenly find old things not working. Also, we revamped the build code and there are minor compiler / TypeScript-ish changes there, so a little extra caution feels warranted.
fongandrew
commented
Apr 10, 2024
| @@ -1,3 +1,7 @@ | |||
| /** | |||
| * @jest-environment jsdom | |||
| */ | |||
There was a problem hiding this comment.
This is admittedly weird, but this directive is needed because @nteract/mockument is loaded before every Jest test (and it's doing a few setup things that even the server version of this package seems to dpeend on).
In an ideal world, we'd probably remove this and put a little more thought into what the server-side rendering environment looks like.
fongandrew
commented
Apr 10, 2024
| this.uCallback = sinon.spy(next); | ||
| this.uCallback(); | ||
| uCallback = sinon.spy(next); | ||
| uCallback(); |
There was a problem hiding this comment.
TypeScript doesn't like this floating this (which ends up referring to window, which is how the tests previously passed).
fongandrew
commented
Apr 10, 2024
| @@ -192,7 +213,9 @@ describe('DOM Events', () => { | |||
| expect(onFocus).toBeTruthy(); | |||
| done(); | |||
| }} | |||
| />); | |||
| />, | |||
| { attachTo: container } | |||
There was a problem hiding this comment.
Focus-related Enzyme tests don't work without being attached to an actual DOM element in the document.
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The immediate goal of this PR is to bump the peer dependency of this to support React 18. Per #293 (and as far as I can tell), it shuld work.
But the build dependencies and CI integration for this package are very out of date, so we had to update a whole bunch of other things just to get here.
rQIb4Vw.mp4
As part of this PR, we also bump the package version to v8. It technically shouldn't be necessary for a major version bump just to support React 18, but this integration is sort of undertested with v18. So we're doing a major version bump to force folks to explicit bump what they have rather than just bringing it in by default and maybe seeing things subtly breaking unexpectedly.