React: False positive getDefaultProps warning when mixing development and production versions

[mondwan]

As migrating from 15.4 to 15.5, I am rewrite original react components with create-react-class.

However, there is a warning.

react-15.5.4.js?bust=1497843639843:3287 Warning: getDefaultProps is only used on classic React.createClass definitions. Use a static property named `defaultProps` instead.

As I am really transferring from a classic React.createClass definition, I would like to ask is this warning appropriate? Or, this is a depreciation sames as the level of migrating to create-react-class?

Ooops, I get ticket #9999 :D

[aweary]

@mondwan can you share an example reproducing the issue? I'm not seeing the warning when using the latest create-react-class version, or the last 15.5 release. Example. You can use this JSFiddle as a starting point for reproducing.

[mondwan]

https://jsfiddle.net/84v837e9/105/

For non minified version, it works as expected. However, it does not if using minified version

Maybe related to issue #9689?

[aweary]

@mondwan the issue is that you're using the minified create-react-class build, but the unminified react build. This means that the internal flag that create-react-class uses to suppress this warning is not applied, so React warns.

Make sure you use the correct build for your environment (minified for production, unminified for development).

[mondwan]

Umm, but why this is a warning at the first place?

2017年6月20日 18:03，"Brandon Dail" [e-mail]寫道：

Closed #9999 https://github.com/facebook/react/issues/9999.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/facebook/react/issues/9999#event-1130677752, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADuAo2St_74neexrI9m8B4o0p6WQ3uXhks5sF5jkgaJpZM4N9x7V
.

[aweary]

Because getDefaultProps is only used for components created with createReactClass. For example, the warning will catch cases where someone is moving from createClass to ES6 classes and they forget to remove getDefaultProps in favor of defaultProps.

You'll never get the warning when using the correct build of create-react-class. It's also important to use minified bundles for react and react-dom in production, as they're much more performant. On the same token, it's useful to use the unminified build in development as it contains a sizeable set of useful checks and warnings.

[mondwan]

That's the point. I am still using react.createClass instead of es6 class
even though this has been replaced by create-react-class. It should not get
a warning at the first place.

Or, create-react-class is made of es6 class. So, rules like that are
effective?

2017年6月20日 18:32，"Brandon Dail" [e-mail]寫道：

Because getDefaultProps is only used for components created with
createReactClass. For example, the warning will catch cases where someone
is moving from createClass to ES6 classes and they forget to remove
getDefaultProps in favor of defaultProps.

You'll never get the warning when using the correct build of
create-react-class. It's also important to use minified bundles for react
and react-dom in production, as they're much more performant. On the same
token, it's useful to use the unminified build in development as it
contains a sizeable set of useful checks and warnings.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/facebook/react/issues/9999#issuecomment-309712579,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADuAow64dLOLEjrIDwGn5DOSqeebBpttks5sF5-lgaJpZM4N9x7V
.

[aweary]

You're getting the warning because you're using the production create-react-class build with the development react build. The production build strips out the code that prevents this warning in development

[gaearon]

In general we don't support mixing production and development versions of libraries.

[gaearon]

It would still be nice if there was a way to avoid the false positive as this is pretty confusing. Maybe we can turn a flag into a number, and always set it in DEV. If it's not set then we're in PROD mode and shouldn't check.

[mondwan]

Ahhh. I see your points. Thanks for the explanations.

Actually, it always show warnings about this problem. However, it is hided
by development codes and those codes get striped when running in production.

In order to get rid of the warnings, I better changes those calls to
defaultProps. Am I correct?

2017年6月21日 下午3:52，"Brandon Dail" [e-mail]寫道：

You're getting the warning because you're using the production
create-react-class build with the development react build. The production
build strips out the code that prevents this warning in development

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/facebook/react/issues/9999#issuecomment-309992470,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADuAo5JvSA_WtxTh_YlhpNCQOTZqDLOlks5sGMvXgaJpZM4N9x7V
.

[gaearon]

No, to fix the false positive warnings, you need to either use:

• react.js and create-react-class.js (development)
• react.min.js and create-react-class.min.js (production)

The mistaken warning occur when you mix development and production versions of these two libraries.

[mondwan]

But it shows no warnings after changing to defaultProps as it suggested.

2017年6月21日 下午6:21，"Dan Abramov" [e-mail]寫道：

No, to fix the false positive warnings, you need to either use:

• react.js and create-react-class.js (development)
• react.min.js and create-react-class.min.js (production)

The mistaken warning occur when you mix development and production
versions of these two libraries.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/facebook/react/issues/9999#issuecomment-310035475,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADuAo0l2pw-Cielxxz32lpFlMcvgjIfUks5sGO62gaJpZM4N9x7V
.

[gaearon]

Both of them work.

But even without changing it, it will still work correctly with createClass. The only issue is mistaken warning. And that mistaken warning happens because you mix development and production versions.

[elCarda]

@gaearon If I do understand it correctly that this can also affect the case when create-react-class is loaded via webpack and minified using uglifyjs during build with whole bundle from sources. The internal token can be named differently -> dozens of false positive warnings in console.

[gaearon]

It is not super clear to me what you mean. Uglify doesn't mangle property names because it is generally unsafe.

[gaearon]

The action item here is to:

1. Reproduce the false positive when mixing development and production versions as described below.
2. Figure out why it happens. You can check create-react-class source code on 15-stable branch. As described above it probably happens because we set some flag only in development.
3. Figure out a fix.

[sportnak]

I'll try looking into this one. I'll update if I'm unable to make any progress. Thanks!

[sportnak]

I've opened a PR for this. react-with-addons.[min].js will warn if isReactClassApproved is null. create-react-class only sets this in development mode, but I think it should be set in both. So I removed the check. Running the test suite shows everything passing as before, so it still warns when it should. But I was uncertain on how to add a production script to a dev build.

Here's the test I used that reproduces the error.

    spyOn(console, 'error');
    var altCreateReactClass = require('./create-react-class.js');
    if (process.env.NODE_ENV !== 'production') {
      altCreateReactClass = require('./create-react-class.min.js');
    }

    var Component = altCreateReactClass({
      getDefaultProps: function() {
        return {
          foo: 0
        };
      },

      render: function() {
        return <span />;
      }
    });

    ReactDOM.render(React.createFactory(Component)(), document.createElement('div'));
    expect(console.error.calls.count()).toBe(0);

[benschac]

@mondwan @gaearon I'd like to grab a good-first-issue. This one looks a bit stale. If it's still needed I'd love to take a crack at it.

[gaearon]

Sure, feel free to look at it. Note https://github.com/facebook/react/pull/12064 is the most obvious fix but it doesn't actually solve the problem: we want to not do any extra steps in production.

[CharlieTruong]

@benschac Are you still poking at this? Would like to take a crack at it if it's not being worked on.

[benschac]

Nah, it's all you. Went into the rabbit hole of chatbots :D

On Sun, Sep 16, 2018 at 3:25 PM, Charlie Truong [e-mail]
wrote:

@benschac https://github.com/benschac Are you still poking at this?
Would like to take a crack at it if it's not being worked on.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/facebook/react/issues/9999#issuecomment-421817173,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACYxIyHoAHEwkH-qRLXtJw9hTYI-ZAuxks5ubqWYgaJpZM4N9x7V
.

[ryansaam]

I would like to take this as my good first issue

[CharlieTruong]

I have a PR open (referenced above your comment) that attempts to address this issue. Awaiting feedback from @gaearon or someone else. Everyone on the core team likely very busy.

[ryansaam]

Yeah I'm sorry I wasn't sure. I should've asked you first. I'm new to contributing so I am still trying to figure out how the flow of things work here. @CharlieTruong

[CharlieTruong]

@ryansaam no worries. I'm new to contributing as well so still trying to figure it out. Feel free to look at my PR and provide feedback. May I approached it wrong.

[garyxuehong]

Hi I made a PR for this issue. The way I did it is adding a flag for dev mode and skip the check if it is not in dev mode.

I need help for writing the test, especially how to test the mix development/release bundle in the test. Much appreciate if anyone can help.

I also has some questions:

1. Should I make PR against branch 15-stable or master?
2. Should I even discuss my changes before I make the PR so it won't wastes someone else's time?

Sorry I am a new comer for react codebase and OSS contribution.

ps: @CharlieTruong , hi my PR is highly inspired from yours. Only thing I changed is to use an explicit flag instead of implicitly rely on the existent of the getDefaultProps function.

[jamesyesware]

@garyxuehong The React contributions docs mention that PRs should always be made against master. Commits are cherrypicked for release branches later on. Looks like your PR is pointing to a stable version branch, though!

[garyxuehong]

@jamesyesware The file I fixed against is in 15-stable branch only (addons/create-react-class/factory.js). The file is not present in master branch. Does that mean this issue doesn't need to be fixed?

[JongminKimSyd]

@jamesyesware and @s-kennedy, it was already explained https://github.com/facebook/react/pull/12064#issuecomment-412979217 by @gaearon .

[george-veras]

Hi! Is this work still needed?
I'm looking to grab a first issue to contribute to, but after spending some time on this thread it seems to have been just left out.

[Stexxe]

When function createClass is executed and there is getDefaultProps in the spec and we are not in production mode, then we approve that getDefaultProps is intentional. As was mentioned before we approve react class only in development mode, because of performance reasons, so we cannot approve it in any environment.

I think the problem is not in mixing development and production versions, but in relying on isReactClassApproved property. The error message says that: getDefaultProps is only used on classic React.createClass definitions, so from logic of the message we should warn only if we have getDefaultProps and element was created not from React.createClass, despite the fact of mixing environments.

The table below describes different states and where bug occurs.

| react | create-react-class | bug |
| --- | --- | --- |
| dev | prod | ✓ (isReactClassApproved is not set) |
| dev | dev | - (isReactClassApproved is set) |
| prod | dev | - (no check) |
| prod| prod | - (no check) |

I have three possible ways in mind of how to fix that bug:

1. We can add extra description of mixing environments issue to warning message to turn the bug into expected behaviour and make it obvious.
2. We can indicate that type was created from React.createClass by adding some property (like __fromCreateClass) to type on any environment and then check that property exists instead of isReactClassApproved.
3. From definition of React.createClass, if the spec has getDefaultProps then we execute that function and set its return value to defaultProps property. We could remove that function from Constructor after filling defaultProps if that won't hurt compatibility with older versions.

The question is how we suppose to fix this if there is no React.createClass in master branch, but only in branch 15.6-dev?