createClass + PropTypes + checkPropTypes warnings #9399

bvaughn · 2017-04-11T00:38:27Z

This PR is prep work for a new 16 alpha release that (temporarily) re-adds getters with deprecation warnings for React.PropTypes, React.checkPropTypes, and React.createClass. I'm hoping to get this new alpha cherry picked into RN 44.

I've broken this PR into multiple commits to make it easier to review:

08bd020: Replace all references to React.PropTypes with prop-types to avoid triggering our own warning message.
ef5b5c6: Removed several references to React.createClass that appeared after rebasing this branch. (reviewed by @flarnie)
524ce20: Added getters for createClass and PropTypes to the main React isomorphic object, behind one-time warning messages. (reviewed by @spicyj)
db48f54: Fixed Rollup bundles to inline 'prop-types' and 'create-react-class' for UMD builds only. (reviewed by @spicyj, @trueadm )
cf49cfd: Updated tests-passing.txt to remove tests that were deleted in this branch.
d34109a: Responses to PR feedback from @spicyj. (Added package.json dependencies to packages/react and packages/react-dom. Renamed a var. Expanded on an inline comment.)
488c8d2: Added warning for moved package to React.checkPropTypes accessor too and updated build script.
83bcb29: Wordsmithing for deprecation notices (added fb.me links).
afdc9d2: Tweaked legacy module inlining to remove order-of-deps constraint
d1348b9: Removed $FlowFixMe.
7dbc3e7: More wordsmithing of deprecation notices based on Dan's feedback.

bvaughn · 2017-04-11T05:03:41Z

Should be ready for review, @acdlite

flarnie · 2017-04-11T16:40:23Z

"Fixed several createClass references in fixtures" lgtm - reviewing the other change now.

…React

… this branch

sophiebits

Reviewed "Added getters" and "Fixed rollup" only. I don't think I fully understand how everything fits together but trust that you do if my questions don't make you unsure. :)

sophiebits · 2017-04-11T17:50:51Z

package.json

@@ -95,6 +93,10 @@
 "node": "4.x || 5.x || 6.x || 7.x",
 "npm": "2.x || 3.x || 4.x"
 },
+ "dependencies": {


Why did you move these? I think dev should be fine. This package.json is only used for development; we have others for the npm packages that get released.

D'oh! Habit. I'll undo this change and update the packages/*/package.json files instead 😄

sophiebits · 2017-04-11T17:53:52Z

scripts/rollup/bundles.js

+ externals: [
+ 'create-react-class/factory',
+ 'prop-types',
+ 'prop-types/checkPropTypes',


Does only isomorphic use checkPropTypes like this? I guess I would have thought that context type checking does too.

DOM does check too but it uses React.checkPropTypes. Rollup also only complained about the externals reference for isomorphic.

sophiebits · 2017-04-11T17:54:36Z

src/isomorphic/React.js

+ Object.defineProperty(React, 'PropTypes', {
+ get() {
+ warning(
+ didWarnPropTypesDeprecated,


nit: inconsistent naming with this and warnedForCreateClass

Let's add something like "it will be removed completely in React 16"

Also, maybe we could make this warning and the one for React.createClass more consistent.

sophiebits · 2017-04-11T17:55:55Z

scripts/rollup/modules.js

@@ -52,6 +52,17 @@ const devOnlyFilesToStubOut = [
 "'ReactTestUtils'",
 ];

+// Ordering of these imports is important;
+// The default import must follow deep imports or Rollup breaks.


Why does Rollup break? (This seems brittle.)

I gave an example below (in the comments) of what it maps the requires to but I'll update it to be a bit more explicit. Basically if it partial-matches one of the keys (eg prop-types/checkPropTypes matches prop-types) then it stops searching and uses a path like prop-types/index.js/checkPropTypes.js

Doesn't this mean that the replacement logic misses ' or something that delimits end of the string, and we should add that?

cc @trueadm

@gaearon I added a comment below, we don't need this logic here :) this should address what @spicyj said as well

1) Added dependencies for create-react-class/prop-types to packages/react*/package.json 2) Renamed a warning let 3) Expanded on an inline comment example

trueadm · 2017-04-11T19:09:04Z

scripts/rollup/modules.js

+ case UMD_DEV:
+ case UMD_PROD:
+ const modulesAlias = {};
+ for (var legacyModule in legacyModules) {


nit: can we use const or let here?

trueadm

I checked over the Rollup still, it all looks good (the nit isn't important)

acdlite · 2017-04-11T19:50:03Z

src/isomorphic/React.js

+ Object.defineProperty(React, 'PropTypes', {
+ get() {
+ warning(
+ didWarnPropTypesDeprecated,


Let's add something like "it will be removed completely in React 16"

Also, maybe we could make this warning and the one for React.createClass more consistent.

acdlite · 2017-04-11T19:53:03Z

src/isomorphic/React.js

+ warning(
+ warnedForPropTypes,
+ 'PropTypes have moved out of the react package. ' +
+ 'Use the prop-types package from npm instead.',


See #9399 (comment)

Suggestion:

PropTypes have been moved to a separate package. Accessing React.PropTypes is no longer supported, and will be removed completely in React 16. Use the prop-types package on npm instead.

And then with a link with more explanation

Sweet. Will do.

… build scripts

bvaughn · 2017-04-11T20:10:35Z

Added wordsmithing suggestions in 83bcb29 @acdlite

trueadm · 2017-04-11T20:12:09Z

scripts/rollup/modules.js

+ case UMD_PROD:
+ const modulesAlias = {};
+ for (var legacyModule in legacyModules) {
+ modulesAlias[legacyModule] = resolve(


As per @gaearon's point above, if we put quotes around this, so it's moduleAlias[`'${legacyModule}'`] then the legacyModule order shouldn't matter as the replace module literally "just" does string replacement :)

Nice! I'll try that. 😁

Changed in afdc9d2.

bvaughn · 2017-04-11T20:26:09Z

Tagging @acdlite to review 488c8d2 and 83bcb29
Tagging @trueadm to review afdc9d2

gaearon · 2017-04-11T20:51:18Z

src/isomorphic/React.js

@@ -106,8 +106,11 @@ if (__DEV__) {
 get() {
 warning(
 warnedForCheckPropTypes,
- 'checkPropTypes has moved out of the react package. ' +
- 'Use the prop-types package from npm instead.',
+ 'checkPropTypes have been moved to a separate package. ' +


Not "has been"?

Words are hard 😅

gaearon · 2017-04-11T20:52:04Z

src/isomorphic/__tests__/React-test.js

 'create-react-class is available on npm as a temporary, ' +
- 'drop-in replacement.',
+ 'drop-in replacement. ' +


Should we make it clear it will keep on working indefinitely? "temporary" doesn't sound very reassuring.

Yeah totally. I'll remove the word "temporary"

**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

* 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 #9232 onto the 15.6 branch. This fixes them. We also needed to add some logic from #9399 **why make this change?:** To keep tests passing and get this change working. **test plan:** `yarn test` **issue:** #9398 * Move component base classes into a single file (#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:** #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`

* 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`

bvaughn requested a review from acdlite April 11, 2017 00:38

facebook-github-bot added the CLA Signed label Apr 11, 2017

bvaughn force-pushed the prop-types branch 4 times, most recently from 2b2237f to 5006ec8 Compare April 11, 2017 04:38

bvaughn changed the title ~~Remove React.PropTypes references in favor of prop-types [WIP]~~ Remove React.PropTypes references in favor of prop-types Apr 11, 2017

bvaughn force-pushed the prop-types branch from 5006ec8 to 005a167 Compare April 11, 2017 05:03

Brian Vaughn added 2 commits April 10, 2017 22:40

Remove React.PropTypes references in favor of prop-types

08bd020

Fixed several createClass references in fixtures

ef5b5c6

bvaughn force-pushed the prop-types branch from 005a167 to ef5b5c6 Compare April 11, 2017 05:41

bvaughn requested a review from flarnie April 11, 2017 16:16

Added createClass and PropTypes getters (w/ depreaction warnings) to …

524ce20

…React

bvaughn changed the title ~~Remove React.PropTypes references in favor of prop-types~~ PropTypes and createClass adjustments Apr 11, 2017

Brian Vaughn added 2 commits April 11, 2017 10:41

Fixed rollup UMD modules

db48f54

Updated Fiber passing tests to remove tests that have been deleted in…

cf49cfd

… this branch

sophiebits reviewed Apr 11, 2017

View reviewed changes

Addressed Ben's PR feedback:

d34109a

1) Added dependencies for create-react-class/prop-types to packages/react*/package.json 2) Renamed a warning let 3) Expanded on an inline comment example

bvaughn requested review from gaearon and trueadm April 11, 2017 18:14

trueadm reviewed Apr 11, 2017

View reviewed changes

trueadm approved these changes Apr 11, 2017

View reviewed changes

bvaughn changed the title ~~PropTypes and createClass adjustments~~ createClass + PropTypes + checkPropTypes warnings Apr 11, 2017

acdlite requested changes Apr 11, 2017

View reviewed changes

acdlite reviewed Apr 11, 2017

View reviewed changes

Added deprecation warning for React.checkPropTypes and updated Rollup…

488c8d2

… build scripts

bvaughn force-pushed the prop-types branch from 1a95456 to 488c8d2 Compare April 11, 2017 20:00

Wordsmithing deprecation notices (and adding fb.me links)

83bcb29

trueadm reviewed Apr 11, 2017

View reviewed changes

Tweaked legacy module inlining to remove order-of-deps constraint

afdc9d2

acdlite approved these changes Apr 11, 2017

View reviewed changes

Removed no-longer-necessary comment

d1348b9

gaearon reviewed Apr 11, 2017

View reviewed changes

More wordsmithing

7dbc3e7

bvaughn merged commit 2beec2f into facebook:master Apr 11, 2017

bvaughn deleted the prop-types branch April 11, 2017 21:28

bvaughn mentioned this pull request Apr 25, 2017

Temporarily add stack target to react-test-renderer bundle #9514

Merged

flarnie mentioned this pull request May 25, 2017

Cherrypick warning and removal of react create class #9771

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

createClass + PropTypes + checkPropTypes warnings #9399

createClass + PropTypes + checkPropTypes warnings #9399

bvaughn commented Apr 11, 2017 •

edited

Loading

bvaughn commented Apr 11, 2017

flarnie commented Apr 11, 2017

sophiebits left a comment

sophiebits Apr 11, 2017

bvaughn Apr 11, 2017

sophiebits Apr 11, 2017

bvaughn Apr 11, 2017

sophiebits Apr 11, 2017

bvaughn Apr 11, 2017

acdlite Apr 11, 2017

sophiebits Apr 11, 2017

bvaughn Apr 11, 2017

gaearon Apr 11, 2017

gaearon Apr 11, 2017

trueadm Apr 11, 2017

trueadm Apr 11, 2017

bvaughn Apr 11, 2017

trueadm left a comment

acdlite Apr 11, 2017

acdlite Apr 11, 2017

acdlite Apr 11, 2017

acdlite Apr 11, 2017 •

edited

Loading

bvaughn Apr 11, 2017

bvaughn commented Apr 11, 2017

trueadm Apr 11, 2017 •

edited

Loading

bvaughn Apr 11, 2017

bvaughn Apr 11, 2017

bvaughn commented Apr 11, 2017

gaearon Apr 11, 2017

bvaughn Apr 11, 2017

gaearon Apr 11, 2017

bvaughn Apr 11, 2017

createClass + PropTypes + checkPropTypes warnings #9399

createClass + PropTypes + checkPropTypes warnings #9399

Conversation

bvaughn commented Apr 11, 2017 • edited Loading

bvaughn commented Apr 11, 2017

flarnie commented Apr 11, 2017

sophiebits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trueadm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Apr 11, 2017

trueadm Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Apr 11, 2017 •

edited

Loading

acdlite Apr 11, 2017 •

edited

Loading

trueadm Apr 11, 2017 •

edited

Loading