react
react copied to clipboard
facebook
False positive getDefaultProps warning when mixing development and production versions
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
@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.
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?
@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).
Umm, but why this is a warning at the first place?
2017年6月20日 18:03，"Brandon Dail" [email protected]寫道：
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 .
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.
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" [email protected]寫道：
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 .
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
In general we don't support mixing production and development versions of libraries.
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.
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" [email protected]寫道：
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 .
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.
But it shows no warnings after changing to defaultProps as it suggested.
2017年6月21日 下午6:21，"Dan Abramov" [email protected]寫道：
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 .
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.
@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.
It is not super clear to me what you mean. Uglify doesn't mangle property names because it is generally unsafe.
The action item here is to:
- Reproduce the false positive when mixing development and production versions as described below.
- Figure out why it happens. You can check
create-react-classsource code on15-stablebranch. As described above it probably happens because we set some flag only in development. - Figure out a fix.
I'll try looking into this one. I'll update if I'm unable to make any progress. Thanks!
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);
@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.
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.
@benschac Are you still poking at this? Would like to take a crack at it if it's not being worked on.
Nah, it's all you. Went into the rabbit hole of chatbots :D
On Sun, Sep 16, 2018 at 3:25 PM, Charlie Truong [email protected] 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 .
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.
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
@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.
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:
- Should I make PR against branch 15-stable or master?
- 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.
@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!
@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?
@jamesyesware and @s-kennedy, it was already explained https://github.com/facebook/react/pull/12064#issuecomment-412979217 by @gaearon .
