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

Cherrypick warning and removal of react create class #9771

Conversation

flarnie
Copy link
Contributor

@flarnie flarnie commented May 25, 2017

Edit: Fixed the rest of the issues, this should be good to go now.

Changes:
cherrypick and hand-edit various parts of commits from master to do the following -

  • add create-react-class and prop-types as dependencies of React
  • add the deprecation warning for React.createClass and get it from the create-react-class package
  • add tests for the React.createClass and React.PropTypes deprecation warnings
  • adds test for create-react-class as used in React

(Original PR message below)

This is a butchered version of #9232 with some of #9399 mixed in. I don't have the tests passing, and there is something not quite right about the way this landed.

  1. Not sure if we want createReactClass or ReactClass.createClass, and updating that caused an error:
  • src/isomorphic/classic/class/createClass.js was added to master in https://github.com/facebook/react/pull/9399/files
  • that file is used in src/isomorphic/React.js and itself uses var {Component} = require('ReactBaseClasses');
  • in 15.6, that line throws Cannot find module 'ReactBaseClasses' from 'createClass.js' and I have no idea why
  • So instead I left it using the old implementation of 'createClass'
  1. There is still a test failing for ReactART and I have not figure out why. Something in ReactReconcileTransaction.perform triggers TypeError: Cannot read property 'call' of undefined.

I'd be happy for any guidance or tips from folks who made those PRs - @acdlite and/or @bvaughn. I am going to delay doing a RC of 15.6 because this is pretty vital to get right.

Still TODO: improve the deprecation message itself.

@flarnie flarnie added this to the 15.6 milestone May 25, 2017
@gaearon
Copy link
Collaborator

gaearon commented May 25, 2017

in 15.6, that line throws Cannot find module 'ReactBaseClasses' from 'createClass.js' and I have no idea why

I moved ReactComponent.js and ReactPureComponent.js into one file on master (#8918) to work around a www issue but didn’t do this for 15. So this is why the file is not found.

@flarnie
Copy link
Contributor Author

flarnie commented May 25, 2017

Thank you @gaearon - I was also tired by the time I got that far. Going to fix this up now, and your hint about that file will help.

@flarnie flarnie force-pushed the cherrypickWarningAndRemovalOfReactCreateClass branch from ba6b6fd to f619d51 Compare May 25, 2017 18:54
yarn.lock Outdated
create-hash "^1.1.0"
inherits "^2.0.1"
ripemd160 "^2.0.0"
safe-buffer "^5.0.1"
sha.js "^2.4.8"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't mean to update all these versions. Will probably make a separate PR with an update to yarn.lock.

acdlite and others added 4 commits May 25, 2017 13:16
**what is the change?:**
A couple of bugs and holes were introduced when cherry-picking facebook#9232 onto the 15.6 branch. This fixes them.
We also needed to add some logic from facebook#9399

**why make this change?:**
To keep tests passing and get this change working.

**test plan:**
`yarn test`

**issue:**
facebook#9398
**what is the change?:**
- Remove some outdated 'require' statements that got orphaned in 'React.js'
- Change 'warning' to 'lowPriorityWarning' for 'React.createClass'
- Fix syntax issues in 'React-test'
- Use 'creatReactClass' instead of ES6 class in ReactART
- Update 'prop-type' dependency to use no higher than 15.7 because 15.8 limits the number of warnings, and this causes a test to fail.
- Fix some mixed-up and misnamed variables in `React.js`
- Rebase onto commit that updates deprecation messages
- Update a test based on new deprecation messages

**why make this change?:**
These were bugs introduced by rebasing and tests caught the regressions.

**test plan:**
`yarn test`

**issue:**
facebook#9398
@flarnie flarnie force-pushed the cherrypickWarningAndRemovalOfReactCreateClass branch from f619d51 to 27bbbec Compare May 25, 2017 20:43
@flarnie flarnie changed the title WIP Cherrypick warning and removal of react create class Cherrypick warning and removal of react create class May 25, 2017
@flarnie
Copy link
Contributor Author

flarnie commented May 25, 2017

Weird - yarn prettier changes nothing locally, and when I try to push git says "Everything up-to-date". So not sure why prettier is failing. I'd like to get CI passing again though.

**what is the change?:**
I didn't mean to commit changes to `yarn.lock` except for the `prop-types` and `create-react-class` updates.

**why make this change?:**
To minimize the changes we make to dependency versions.

**test plan:**
`rm -rf node_modules`
`yarn install`
`yarn run build`
`yarn test`
@flarnie flarnie mentioned this pull request May 25, 2017
49 tasks
Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

Why did create-react-class and prop-types move from devDependencies to dependencies?

@flarnie
Copy link
Contributor Author

flarnie commented May 25, 2017

In #9399 we swap out our own React.createClass for the create-react-class package under the hood, and do the same for React.PropTypes, delegating to prop-types.
It looks like we did the prop-types delegation before 15.6? So not sure if it's a problem in earlier versions that prop-types is a devDependency.
It should get turned into a no-op in production though, right?
Might be worth looking more at how this affects different builds.

But in general - React is actually using those packages until we remove those APIs. Also I added a call to createReactClass to ReactART to fix a cryptic error. Maybe we don't need prop-types but I was copying from #9399

@flarnie
Copy link
Contributor Author

flarnie commented May 25, 2017

Just looked and I think it seems safer to keep prop-types as a dependency in this version. We could do some extra stuff to make it dev only in this version, but since we will be removing it from React in 16.0 I don't think it's worth any extra effort at this point.

@flarnie flarnie merged commit b48b259 into facebook:15.6-dev May 25, 2017
@sophiebits
Copy link
Collaborator

👍 I don't think it makes a difference. As far as I know we don't ever do a non-dev install of the root package.json's dependencies.

@flarnie
Copy link
Contributor Author

flarnie commented May 25, 2017

@gaearon - b43f830 removes the // es6 comment.

@Wilfred
Copy link

Wilfred commented May 26, 2017

Where is the source code of create-react-class? https://www.npmjs.com/package/create-react-class claims that the source code is in this repo, but this repo seems to be depending on the create-react-class package.

@gaearon
Copy link
Collaborator

gaearon commented May 26, 2017

It is in the 15.6-dev branch.

@Wilfred
Copy link

Wilfred commented May 26, 2017

Thanks. For future reference, it's here: https://github.com/facebook/react/tree/15.6-dev/addons/create-react-class

@gaearon
Copy link
Collaborator

gaearon commented May 26, 2017

The latest released version is always in 15-stable.

flarnie added a commit to flarnie/react that referenced this pull request Jun 7, 2017
* react-create-class -> create-react-class

* Fix issues/bugs introduced by merge conflict resolution

**what is the change?:**
A couple of bugs and holes were introduced when cherry-picking facebook#9232 onto the 15.6 branch. This fixes them.
We also needed to add some logic from facebook#9399

**why make this change?:**
To keep tests passing and get this change working.

**test plan:**
`yarn test`

**issue:**
facebook#9398

* Move component base classes into a single file (facebook#8918)

* More fixes for issues introduced by rebasing

**what is the change?:**
- Remove some outdated 'require' statements that got orphaned in 'React.js'
- Change 'warning' to 'lowPriorityWarning' for 'React.createClass'
- Fix syntax issues in 'React-test'
- Use 'creatReactClass' instead of ES6 class in ReactART
- Update 'prop-type' dependency to use no higher than 15.7 because 15.8 limits the number of warnings, and this causes a test to fail.
- Fix some mixed-up and misnamed variables in `React.js`
- Rebase onto commit that updates deprecation messages
- Update a test based on new deprecation messages

**why make this change?:**
These were bugs introduced by rebasing and tests caught the regressions.

**test plan:**
`yarn test`

**issue:**
facebook#9398

* Reset `yarn.lock`

**what is the change?:**
I didn't mean to commit changes to `yarn.lock` except for the `prop-types` and `create-react-class` updates.

**why make this change?:**
To minimize the changes we make to dependency versions.

**test plan:**
`rm -rf node_modules`
`yarn install`
`yarn run build`
`yarn test`

* Run `yarn prettier`
@flarnie flarnie deleted the cherrypickWarningAndRemovalOfReactCreateClass branch May 25, 2018 17:51
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.

None yet

6 participants