Remove createClass #9232

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

src/isomorphic/React.js

+    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

src/isomorphic/React.js

+    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

@spicyj replaceState 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-24T21:19:17Z

So here's what I'll do: Implement replaceState and isMounted within react-create-class itself with "close-enough" semantics. Warn once for isMounted usage since it's slightly riskier and more of an anti-pattern. Don't warn for replaceState because it's probably fine.

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. facebook/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 facebook#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:** facebook#9398

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

The `React.createClass` is going to be deprecated soon and will be removed in React v16. facebook/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.createClass remarkjs/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 8708a07 Compare 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 ed00197 Compare 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 957fbc9 Compare April 11, 2017 00:06

acdlite merged commit 957fbc9 into facebook:master Apr 11, 2017

flarnie mentioned this pull request May 25, 2017

Cherrypick warning and removal of react create class #9771

Merged

Remove createClass #9232

Remove createClass #9232

Conversation

acdlite commented Mar 22, 2017

gaearon Mar 22, 2017

Choose a reason for hiding this comment

acdlite Mar 22, 2017

Choose a reason for hiding this comment

gaearon Mar 22, 2017

Choose a reason for hiding this comment

nhunzaker commented Mar 22, 2017

kentcdodds commented Mar 22, 2017 • edited Loading

bvaughn left a comment

Choose a reason for hiding this comment

bvaughn Mar 22, 2017

Choose a reason for hiding this comment

aweary Mar 22, 2017

Choose a reason for hiding this comment

bvaughn Mar 22, 2017

Choose a reason for hiding this comment

acdlite commented Mar 22, 2017

sophiebits commented Mar 24, 2017

acdlite commented Mar 24, 2017

acdlite commented Mar 24, 2017

sophiebits commented Mar 24, 2017

acdlite commented Mar 24, 2017

sophiebits commented Mar 24, 2017

acdlite commented Mar 24, 2017

sophiebits commented Mar 24, 2017

bvaughn commented Mar 24, 2017

acdlite commented Mar 24, 2017 • edited Loading

bvaughn commented Mar 24, 2017

sophiebits commented Mar 24, 2017

acdlite commented Mar 24, 2017

bvaughn commented Mar 24, 2017

acdlite commented Mar 24, 2017

bvaughn commented Mar 24, 2017

gaearon commented Mar 24, 2017

bvaughn commented Mar 24, 2017 • edited Loading

acdlite commented Mar 24, 2017

gaearon commented Mar 24, 2017

acdlite commented Mar 24, 2017

acdlite commented Mar 24, 2017

sophiebits commented Mar 24, 2017

acdlite commented Mar 24, 2017

acdlite commented Mar 24, 2017

acdlite commented Mar 27, 2017

bvaughn commented Mar 27, 2017

kentcdodds commented Mar 22, 2017 •

edited

Loading

acdlite commented Mar 24, 2017 •

edited

Loading

bvaughn commented Mar 24, 2017 •

edited

Loading