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

RFC 6: createReactClass updates #12036

Merged
merged 6 commits into from
Jan 19, 2018
Merged

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jan 17, 2018

This codemod corresponds with reactjs/rfcs/pull/6 and /pull/12028

Make create-react-class factory aware of the new "UNSAFE_" lifecycle names so that:

  • It warns about spelling error for UNSAFE_componentWillRecieveProps.
  • It doesn't trigger an invariant if one of the new lifecycle names are mixed in as overrides.

I've built addons/create-react-class locally and compared it to the 15.6.2 release in NPM to verify that the only changes between the two are the additions I've made with this PR. This should make the release very low risk.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 17, 2018

CI failure is unexpected, given the limited scope of these changes.

screen shot 2018-01-17 at 3 00 38 pm

Edit: Looks like the last commit to this branch (4c75c5b) also failed CI for this same reason, so I'm going to ignore it.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 17, 2018

I don't think I actually need to care about the failing CI. The changes to the bundled create-react-class package are so minimal, it should be easy to verify them by hand when doing the NPM release.

@sebmarkbage
Copy link
Collaborator

What about the static one getDerivedStateFromProps? Do we not it to merge similarly as other mixin things?

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 18, 2018

Statics can't be defined on the object passed to the create-react-class factory, can they?

I've added tests for the combination of these two with 286df77 FWIW.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Jan 18, 2018

createClass({
  statics: {
    getDerivedStateFromProps() {
    }
  }
})

This approach also works with mixins.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 18, 2018

Oh cool. I had no idea. 😄

Declaring the static method should work without any changes, although I've added a test for it (68f2fe7).

Mixing it in wouldn't, but I think that's reasonable since...what would it mean to mix two of those methods? What would the return value be?

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 18, 2018

This should be ready for review 😄

I'm happy to publish to NPM once it's merged. I've done a dry-run locally and verified that all of the tests for this addon pass and no unexpected changes are in the build.

@gaearon
Copy link
Collaborator

gaearon commented Jan 18, 2018

Mixing it in wouldn't, but I think that's reasonable since...what would it mean to mix two of those methods? What would the return value be?

Should work the same way as getInitialState is already being merged.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 18, 2018

Should work the same way as getInitialState is already being merged.

Eh, yuck. 😄 Okay.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 19, 2018

I've added support for static getDerivedStateFromProps to be mixed in. I also added tests for this as well as the getInitialState instance mixin.

*
* @optional
*/
UNSAFE_shouldComponentUpdate: 'DEFINE_MANY',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new to me. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. Sorry! Looks like I fucked up there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha no worries. Fresh eyes always help 👀

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me

@bvaughn bvaughn merged commit 5530c75 into facebook:15.6-dev Jan 19, 2018
@bvaughn bvaughn deleted the 15.6-dev-rfc-6 branch January 19, 2018 23:13
@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 19, 2018

Thanks!

Releasing this should be pretty simple. (Just run yarn build from within the addons/create-react-class directory and compare to the latest release in NPM.)

I'll wait and release it just before the 16.3 release though, so as to avoid confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants