Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-add the warning about PropTypes #8080

Closed
gaearon opened this issue Oct 24, 2016 · 5 comments
Closed

Re-add the warning about PropTypes #8080

gaearon opened this issue Oct 24, 2016 · 5 comments
Assignees
Milestone

Comments

@gaearon
Copy link
Collaborator

gaearon commented Oct 24, 2016

After a discussion with @spicyj we decided to keep the warning even though the functions will throw in production. The warning is the only way to learn about that.

We should also document how to pass the secret through in case you know what you're doing.

@gaearon gaearon self-assigned this Oct 24, 2016
@gaearon gaearon added this to the 16.0 milestone Oct 24, 2016
@sophiebits
Copy link
Collaborator

even though the functions will throw in production

particularly because the functions will throw in production

@stevewillard
Copy link

Out of curiosity, why will they throw errors in 16.0?

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 28, 2016

Because we will eliminate all that code in production. It doesn't seem to make sense to ship real code for PropTypes validators if it's normally never used in prod. Therefore we'll replace them all with an empty function. To make this behavior clear we'll make it throw so people don't rely on them for validating e.g. user input.

@thysultan
Copy link

You could also create PropTypes on demand, for example

Object.defineProperties(api, {'PropTypes': {
    // only construct PropTypes when used
    get: function () {
        return (
            Object.defineProperty(
                this, 
                'PropTypes', 
                { value: generatePropTypes() }
            ),
            this.PropTypes
        );
    },
    configurable: true, 
    enumerable: true
....

@Jessidhia
Copy link
Contributor

That would defeat the point by including even more code, rather than making the production bundle smaller.

gaearon added a commit to gaearon/react that referenced this issue Jan 31, 2017
…k#7132)""

This reverts commit be71f76.

In other words, now we again have the warning if you attempt to call PropTypes manually.
It was removed in facebook#8066 but we shouldn't have done this since we still want to avoid people accidentally calling them in production (and even more so since now it would throw).

Fixes facebook#8080.
gaearon added a commit to gaearon/react that referenced this issue Feb 7, 2017
…k#7132)""

This reverts commit be71f76.

In other words, now we again have the warning if you attempt to call PropTypes manually.
It was removed in facebook#8066 but we shouldn't have done this since we still want to avoid people accidentally calling them in production (and even more so since now it would throw).

Fixes facebook#8080.
gaearon added a commit that referenced this issue Feb 8, 2017
* Revert "Revert "Warn if PropType function is called manually (#7132)""

This reverts commit be71f76.

In other words, now we again have the warning if you attempt to call PropTypes manually.
It was removed in #8066 but we shouldn't have done this since we still want to avoid people accidentally calling them in production (and even more so since now it would throw).

Fixes #8080.

* Pass secret in ReactControlledValuePropTypes

* Record tests
@bvaughn bvaughn mentioned this issue Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants