Remove createClass by acdlite · Pull Request #9232 · react/react

acdlite · 2017-03-22T01:47:15Z

Tests that rely on replaceState or isMounted use updater.enqueueReplaceState and updater.isMounted instead.

Won't be released until 16. In 15.5 we'll deprecate and point users to an external module, react-create-class. After than we'll continue to warn but it will no longer be accessible via the main React package.

gaearon · 2017-03-22T12:29:52Z

+ Object.defineProperty(React, 'createClass', {
+ get: function() {
+ warning(
+ warnedForCreateClass,


What do you think about waiting for first call and including the class displayName in the message?

We can do that in 15.5 (I have a separate branch that I haven't pushed to yet) but in 16 React.createClass is undefined. We could use a void function and warn in there but that would lead to confusing errors in prod.

nhunzaker · 2017-03-22T13:07:13Z

Very cool! @aweary I'll make sure this doesn't conflict with our Dom fixtures.

kentcdodds · 2017-03-22T13:29:52Z

In case anyone else is curious, here's the relevant compare_size:files output that compares filesize of master to this pull request:

 raw gz Compared to master @ 8ce5fe97b5251c480d0a4246f2c6dcbfad9ba1f1 = = build/react-dom-fiber.js = = build/react-dom-fiber.min.js = = build/react-dom-server.js = = build/react-dom-server.min.js = = build/react-dom.js = = build/react-dom.min.js -25406 -6086 build/react.js -3428 -1052 build/react.min.js

So the gzipped and minified version of react (the size most people should be shipping) is a whole 1kb smaller with this change. 🎉

For more context, here are the full sizes from master:

 raw gz Sizes 645638 142625 build/react-dom-fiber.js 133023 41361 build/react-dom-fiber.min.js 572061 131083 build/react-dom-server.js 122251 37292 build/react-dom-server.min.js 928774 205399 build/react-dom.js 133088 41393 build/react-dom.min.js 132348 31265 build/react.js 16876 6152 build/react.min.js

And the size from this PR:

 raw gz Sizes 645638 142625 build/react-dom-fiber.js 133023 41361 build/react-dom-fiber.min.js 572061 131083 build/react-dom-server.js 122251 37292 build/react-dom-server.min.js 928774 205399 build/react-dom.js 133088 41393 build/react-dom.min.js 106942 25179 build/react.js 13448 5100 build/react.min.js

So that 1kb drop in the minified/gzipped react represents a 17% drop in the overall size. This is not insignificant! :party_parrot:

Great work @acdlite 👏

Edit: @aickin pointed out that most people should probably care about the react.js and react.dom.js combined size. Taking that into account, this is more like a 2.3% drop. Still super significant! Yay!

bvaughn

This looks fantastic! So exciting to see all that deleted code and the file size reductions Kent mentions above. 😁

bvaughn · 2017-03-22T17:51:05Z

+ return mixin;
+ };
+
+ if (canDefineProperty) {


nit: Curious why not move canDefineProperty and warning require statements into this __DEV__ block so you can get rid of this check? They're only used here.

Isn't the check there to see if you can use Object.defineProperty though?

😆 Yes, ignore me.

acdlite · 2017-03-22T20:14:13Z

I'll wait to merge this until we've updated the createClass codemod to fallback to importing from react-create-class, in the case where it can't convert to a plain JavaScript class.

sophiebits · 2017-03-24T19:58:48Z

Can we implement replaceState and isMounted in user land then kill them on the updater? I don't want to have to maintain those as public API.

acdlite · 2017-03-24T20:02:08Z

@spicyj What was the reason for removing them in the first place?

acdlite · 2017-03-24T20:03:51Z

@spicyj On that point, we should obscure the updater property. Can deprecate access in 15.5.

sophiebits · 2017-03-24T20:05:13Z

replaceState is noncompositional and makes it harder to add new state properties to a component; isMounted is often better done as a subscription that gets cancelled.

acdlite · 2017-03-24T20:06:44Z

@spicyj I'm cool with killing isMounted. Indifferent on replaceState.

sophiebits · 2017-03-24T20:07:47Z

My point is, replaceState and isMounted are only supported on createClass so we should have the createClass implementation handle them internally rather than keeping logic forever in React proper to support them.

acdlite · 2017-03-24T20:15:56Z

@spicyjreplaceState is pretty low overhead to maintain in React, more complicated to implement externally and we'd still be tied to the internals, so not sure it reduces the risk of breakage in the future. Even more so with isMounted — the implementation is not trivial and heavily tied to internal data structures.

I figured we would keep them around for a few more releases and then when it breaks for react-create-class users... oh well, we warned them.

sophiebits · 2017-03-24T20:19:03Z

I meant rewrite them without using internals: for replaceState, setting all existing keys to undefined is probably close enough; for isMounted, can we not replicate with lifecycle methods? Ideally folks could use react-create-class forever, not until "oh well we broke it".

bvaughn · 2017-03-24T20:20:10Z

Even more so with isMounted — the implementation is not trivial and heavily tied to internal data structures.

Wouldn't this be as simple as setting an instance prop in componentDidMount and unsetting it in componentWillUnmount?

acdlite · 2017-03-24T20:24:50Z

I don't think it's exactly the same. What if you call isMounted on a child during the unmount cycle? Child has already called componentWillUnmount, so it returns true even though it hasn't actually been unmounted yet.

But maybe this is "close enough" to not matter. Not sure.

bvaughn · 2017-03-24T20:27:38Z

I don't think it's exactly the same. What if you call isMounted on a child during the unmount cycle?

Yeah, I didn't think that case is important to support.

sophiebits · 2017-03-24T20:28:18Z

I am happier breaking that now then forgetting about it forever.

acdlite · 2017-03-24T20:31:17Z

If we do this, I think we should warn... at least in isMounted.

bvaughn · 2017-03-24T20:33:48Z

What if we only warned the first time isMounted getter was called during the unmount lifecycle?

acdlite · 2017-03-24T20:40:13Z

@bvaughn Are you concerned about too many warnings?

bvaughn · 2017-03-24T20:42:57Z

Not really. If we only warn...

in DEV mode
if isMounted is called during the unmount cycle
the first time

I think we're unlikely to bother many people.

gaearon · 2017-03-24T20:43:04Z

Can we realistically break anything with this slight change in behavior? Are we making isMounted more paranoid? If so, is this breaking?

bvaughn · 2017-03-24T20:48:00Z

I think it's unlikely. If you're doing DOM cleanup (eg removing event listeners) in componentWillUnmount then you know you're about to be unmounted and you'd have no reason to check. I guess it's possible you'd trigger a helper method written to be called at any time...

~~I could write a script that checks to see if isMounted is ever called synchronously during the unmount cycle if we'd like to determine if Facebook ever does this?~~ Actually maybe that would be harder than I initially thought. I could at least scan through visually if we'd like. There aren't that many calls.

acdlite · 2017-03-24T20:51:48Z

@bvaughn No I meant to ask, why not warn on every usage? It's an anti-pattern, anyway. Then the user is directed to either find a better solution or implement a componentDidMount flag themselves.

gaearon · 2017-03-24T20:52:53Z

If it's in a library code then warning on every usage will spam the console unactionably. I think people had enough bad experience with the unknown DOM property warning that we should be very careful with adding repeated warnings.

acdlite · 2017-03-24T22:37:54Z

I'm going to warn for replaceState as well. I forgot about the use case where people call replaceState(customDataType), like for Immutable.js. This actually seems like more of a break than the isMounted case. Not a huge deal, but I'm going to warn on every usage.

sophiebits · 2017-03-24T22:40:27Z

I thought we had a warning for that already maybe? As long as you mean just "warn on every non-plain-object" I guess that's okay.

acdlite · 2017-03-24T22:42:23Z

@spicyj Oh maybe we do, wasn't aware!

acdlite · 2017-03-24T23:03:23Z

Turns out we have a warning for props but not state

acdlite · 2017-03-27T20:45:56Z

Update: The Immutable JS case (preserving the prototype when calling replaceState) is the main reason people still use it, so we'll just keep using updater.enqueueReplaceState. It's somewhat likely that replaceState will be the default semantics for a functional component API, anyway, and it's not difficult to maintain. Worst-case scenario, if it breaks in the future, we can do another release of react-create-class.

bvaughn · 2017-03-27T23:40:03Z

Corresponding update to transform-react-display-name via babel/babel/pull/5554

The `React.createClass` is going to be deprecated soon and will be removed in React v16. react/react#9232 This patch changes the component to a class extended from `React.Component`. It does it manually by assigning to the prototype, to avoid introducing a build step.

Remove createClass from isomorphic package

instead of createClass

Tests that rely on replaceState or isMounted use updater.enqueueReplaceState and updater.isMounted instead.

Should still be undefined.

This snuck in after rebasing

These are mostly copied from the old ReactClass-test module.

**what is the change?:** NOTE: There are still bugs here, we will need at least a few more fixes. A couple of bugs and holes were introduced when cherry-picking react#9232 onto the 15.6 branch. This fixes them. **why make this change?:** To keep tests passing and get this change working. **test plan:** `yarn test` **issue:** react#9398

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

The `React.createClass` is going to be deprecated soon and will be removed in React v16. react/react#9232 This patch changes the component to a class extended from `React.Component`. It does it manually by assigning to the prototype, to avoid introducing a build step.

acdlite requested a review from bvaughn March 22, 2017 01:47

facebook-github-bot added the CLA Signed label Mar 22, 2017

NE-SmallTown mentioned this pull request Mar 22, 2017
Support React 16 ant-design/ant-design#5377
Closed
27 tasks

gaearon reviewed Mar 22, 2017
View reviewed changes

goto-bus-stop mentioned this pull request Mar 22, 2017
Remove React.createClassremarkjs/react-markdown#66
Merged

bvaughn approved these changes Mar 22, 2017
View reviewed changes

DanielMSchmidt approved these changes Mar 23, 2017
View reviewed changes

acdlite force-pushed the removecreateclass branch 2 times, most recently from 8795b7b to 8708a07Compare March 27, 2017 23:35

bvaughn mentioned this pull request Mar 27, 2017
Updated transform-react-display-name for createReactClass addon babel/babel#5554
Merged

neeharv mentioned this pull request Apr 5, 2017
Collaborators joshwnj/react-visibility-sensor#72
Open

acdlite force-pushed the removecreateclass branch from 8708a07 to ed00197Compare April 7, 2017 22:17

acdlite added 8 commits April 10, 2017 17:03

Delete createClass
5cfaa7c
Remove createClass from isomorphic package

Rewrite Stack implementation of ReactART using plain classes
087fe88
instead of createClass

Update tests to use plain JavaScript classes
c76a4ea
Tests that rely on replaceState or isMounted use updater.enqueueReplaceState and updater.isMounted instead.

Run prettier
194b25f

Warn once when attempting to access React.createClass
646e786
Should still be undefined.

Convert createClass caller
6b539f2
This snuck in after rebasing

Add react-create-class integration tests
ce48926
These are mostly copied from the old ReactClass-test module.

react-create-class -> create-react-class
957fbc9

acdlite force-pushed the removecreateclass branch from ed00197 to 957fbc9Compare April 11, 2017 00:06

acdlite merged commit 957fbc9 into react:masterApr 11, 2017

flarnie mentioned this pull request May 25, 2017
Cherrypick warning and removal of react create class #9771
Merged

Uh oh!

Conversation

acdlite commented Mar 22, 2017

Uh oh!

gaearonMar 22, 2017

Choose a reason for hiding this comment

Uh oh!

acdliteMar 22, 2017

Choose a reason for hiding this comment

Uh oh!

gaearonMar 22, 2017

Choose a reason for hiding this comment

Uh oh!

nhunzaker commented Mar 22, 2017

Uh oh!

kentcdodds commented Mar 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn left a comment

Choose a reason for hiding this comment

Uh oh!

bvaughnMar 22, 2017

Choose a reason for hiding this comment

Uh oh!

awearyMar 22, 2017

Choose a reason for hiding this comment

Uh oh!

bvaughnMar 22, 2017

Choose a reason for hiding this comment

Uh oh!

acdlite commented Mar 22, 2017

Uh oh!

sophiebits commented Mar 24, 2017

Uh oh!

acdlite commented Mar 24, 2017

Uh oh!

acdlite commented Mar 24, 2017

Uh oh!

sophiebits commented Mar 24, 2017

Uh oh!

acdlite commented Mar 24, 2017

Uh oh!

sophiebits commented Mar 24, 2017

Uh oh!

acdlite commented Mar 24, 2017

Uh oh!

sophiebits commented Mar 24, 2017

Uh oh!

bvaughn commented Mar 24, 2017

Uh oh!

acdlite commented Mar 24, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn commented Mar 24, 2017

Uh oh!

sophiebits commented Mar 24, 2017

Uh oh!

acdlite commented Mar 24, 2017

Uh oh!

bvaughn commented Mar 24, 2017

Uh oh!

acdlite commented Mar 24, 2017

Uh oh!

bvaughn commented Mar 24, 2017

Uh oh!

gaearon commented Mar 24, 2017

Uh oh!

bvaughn commented Mar 24, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acdlite commented Mar 24, 2017

Uh oh!

gaearon commented Mar 24, 2017

Uh oh!

acdlite commented Mar 24, 2017

Uh oh!

sophiebits commented Mar 24, 2017

Uh oh!

acdlite commented Mar 24, 2017

Uh oh!

kentcdodds commented Mar 22, 2017•
edited
Loading

acdlite commented Mar 24, 2017•
edited
Loading

bvaughn commented Mar 24, 2017•
edited
Loading