Upgrade to latest react #24
Conversation
package.json
Outdated
| }, | ||
| "peerDependencies": { | ||
| "react": "^0.14.0 || ^15.0.0-0 || ^16.0.0-0" | ||
| "react": "^16.0.0-0" |
There was a problem hiding this comment.
Should we have tests for each of these versions?
There was a problem hiding this comment.
If it's possible to continue to support the older versions of React, I think that'd be nice and then yeah we'd probably want to test them. Not sure how difficult it would be to continue to support the older versions especially with renamed APIs like UNSAFE_componentWillReceiveProps—I also think it would be fine to drop support and call this a breaking change.
There was a problem hiding this comment.
IMO calling this a breaking change and supporting v16 is the way to go. Not sure if supporting older versions of React will have a significant benefit but as we keep improving (hooks, context providers), we won't support the older versions anyways.
There was a problem hiding this comment.
Feels weird to release a new major version that doesn't break backwards compatibility yet.
I'm fine either merging this in as is but wait to publish until we introduce actual breaking changes
or
back this change out and just release as a minor version update.
There was a problem hiding this comment.
UNSAFE_ was introduced with v16.3 I believe, so with any older version of react, it will not work. Thus my "react": "^16.0.0-0" is actually wrong as well and it should be a minimum v16.3. The library tests fails with older versions of react and even with v16.0. (I need to update the peer dependency version to v16.3)
If we won't be publishing it, then I do not see the benefit of merging this. It would be better to do this as a part of an improvement (such as adding hooks) and not just dependency version update but I am not sure if that would be useful. We can focus on the PR by @ghmeier and do these changes along with that and release that.
v0.14 || v15.0 will not work so we can not change this out and release a minor version
There was a problem hiding this comment.
So the other other option is to release this as a major version with these changes? I'm ok with that too. I agree with your reasoning.
There was a problem hiding this comment.
So let's do this as a major release. I will go ahead and merge the PR. Do we have documentation for the release process?
There was a problem hiding this comment.
I don't have permissions :) Feel free to merge and release if it's all good.
| assert.equal(userModel.get('name'), newName); | ||
| assert.equal(stub.props().name, newName); | ||
| assert.equal(wrapper.find(TestComponent).prop('name'), newName); |
There was a problem hiding this comment.
Good to know that this is because something changed in enzyme rather than something new in react.
| }); | ||
|
|
||
| it('should return the wrapped component via getWrappedInstance()', function() { | ||
| assert.equal(wrapper.instance().getWrappedInstance(), stub.node); | ||
| assert.equal(wrapper.instance().getWrappedInstance(), stub.instance()); |
| @@ -96,7 +96,7 @@ describe('connectBackboneToReact', function() { | |||
| }); | |||
|
|
|||
| afterEach(function() { | |||
| wrapper.unmount(); | |||
| if (wrapper.exists()) wrapper.unmount(); | |||
There was a problem hiding this comment.
Why is this now required? What changed? Seems like a bandaid when there is some other underlying issue at hand?
There was a problem hiding this comment.
So there are 2 tests which unmounts inside the test so when the afterEach tries to run the unmount again it fails. That's why I will just keep this if check in the afterEach and I have removed from the individual tests.
When removed 2 tests results in an error Error: Method “unmount” is meant to be run on 1 node. 0 found instead. we have upgraded to 3.10 and I found this in the enzyme issues enzymejs/enzyme#1880
package.json
Outdated
| }, | ||
| "peerDependencies": { | ||
| "react": "^0.14.0 || ^15.0.0-0 || ^16.0.0-0" | ||
| "react": "^16.0.0-0" |
There was a problem hiding this comment.
Feels weird to release a new major version that doesn't break backwards compatibility yet.
I'm fine either merging this in as is but wait to publish until we introduce actual breaking changes
or
back this change out and just release as a minor version update.
|
Published! |
|
Thank you everyone!! |
We want to start using React v16.12 in some of our projects that is using the connect-backbone-to-react. Even though upgrading the react version does not cause any problems for our application, we still want to validate that all the tests for the CBR passes with the latest version.
Big shoutout to @ghmeier for his PR :) My initial idea was to fix his PR which uses context providers but that would also require changes in our application. That's why I have decided to create this PR.