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

Warn if PropType function is called manually #7132

Merged
merged 14 commits into from
Jul 5, 2016

Conversation

aweary
Copy link
Contributor

@aweary aweary commented Jun 27, 2016

Resolves #7131

@gaearon so I added a new file ReactPropTypesSecret that is used internally to verify that a propType function is not being called manually. For the tests I basically reused what you had in #6401 as far as structure goes. Everything should be passing.

I wasn't sure if we wanted to dedupe this errors. If so I'm thinking we'd have to track it by componentName or something. Let me know what you think.

) {
componentName = componentName || ANONYMOUS;
propFullName = propFullName || propName;
if (!__DEV__) {
if (secret !== ReactPropTypesSecret) {
console.error(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s also check if (typeof console !== 'undefined')

'testComponent',
'prop'
);
const expectedCount = __DEV__ ? 0 : 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s make an if branch and do different assertions in if and else. This would read simpler IMO.

Copy link
Contributor Author

@aweary aweary Jun 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if we're not doing any __DEV__ tests then it would just be a constant 0 right? No need for any kind of branching.


'use strict';

const ReactPropTypesSecret = '__REACT_PROP_TYPES_SECRET__' + Math.random();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: .toString().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 will do. Just curious, why? It'll be coerced to a string either way AFAIK.

@gaearon
Copy link
Collaborator

gaearon commented Jun 27, 2016

My concern was that if we only warned once and they are calling PropType functions in multiple places they are only being warned about one of those instances. That might be annoying, especially in production, since you'd think you fixed it (since you only got one warning) and then you'd get the warning again for the next instance.

Now that I think of it, why not warn in development instead? It would be much easier to catch that issue even if the warning is only printed once.

Sorry about the churn 😄

So we could do the following:

  • Warn once on any manual call in development
  • Don’t do anything in production

Do you think this makes more sense?

We’d still change production behavior eventually, but at least people would become aware during development that calling them manually is not OK—whether in development or in production.

@aweary
Copy link
Contributor Author

aweary commented Jun 27, 2016

Now that I think of it, why not warn in development instead? It would be much easier to catch that issue even if the warning is only printed once.

I agree, production-only errors might be frustrating since some users might not catch those until it's deployed to a staging environment (or even to actual production).

Sorry about the churn 😄

No worries at all! I'd rather get it done right than get it done quick 😄

So we could do the following:

  • Warn once on any manual call in development
  • Don’t do anything in production

That seems sensible and consistent with other warnings/errors in React. Since we'd be doing these warnings in development, I think it would make the most sense to warn once for every instance where they're calling PropTypes function manually. Since dev warnings aren't really expensive, I feel like that would be the most useful.

All we'd need to do is create a cache in the createChainableTypeChecker closure that stores the component and prop name (like { component { prop } }) and check against that.

Does that sound good @gaearon?

@gaearon
Copy link
Collaborator

gaearon commented Jun 27, 2016

I think it would make the most sense to warn once for every instance where they're calling PropTypes function manually.

I’m hesitant because I can’t imagine somebody calling them as one-offs. If somebody actually calls those functions manually, I imagine they might be a part of something like a form validator, in which case you’d see a warning on every keystroke.

Do you see deduping based on componentName:propName a good compromise?

@aweary
Copy link
Contributor Author

aweary commented Jun 27, 2016

Do you see deduping based on componentName:propName a good compromise?

That's actually what I was trying to recommend. Checking if a warning has been logged for ${component}:${prop} so that it's not logged multiple times if props are checked again, but it still logs once for every instance where they are manually calling the PropType function.

By "instance" I meant every place where they're manually calling a PropType function, not every time the function is invoked. Sorry, my terminology was probably a little confusing.

@gaearon
Copy link
Collaborator

gaearon commented Jun 28, 2016

Well, technically, you can’t really know where it’s being called from. You only know what arguments are being passed. But seems like we agree on the proposal.

@aweary
Copy link
Contributor Author

aweary commented Jun 28, 2016

Well, technically, you can’t really know where it’s being called from. You only know what arguments are being passed.

Right, I don't mean to imply that the location in the code path is being considered at all. I mean that we cache it so that for each PropTypes function we only log out a warning once for any combination of componentName or propName, regardless of how many times it might be invoked with those arguments.

But seems like we agree on the proposal.
👍

@aweary
Copy link
Contributor Author

aweary commented Jul 5, 2016

@gaearon I'd be happy to! I'll get a PR going right now

usmanajmal pushed a commit to usmanajmal/react that referenced this pull request Jul 11, 2016
* Warn if PropType function is called in production

* Check if console is undefined before warning

* Randomize value of ReactPropTypesSecret

* Remove dev environment tests

* Rename typeCheckPass to productionWarningCheck

* Rename productionWarningCheck to expectWarningInProduction

* Call toString on Math.random()

* Rename test block for React type checks

* Make sure warning isnt emitted for failing props

* Cache warning by component and prop, warn in dev

* Pass ReactPropTypesSecret to internal checks

* Move tests to ReactPropTypes-test.js

* Update the warning message to include link

* Do not test warning for unions  with invalid args
@zpao zpao modified the milestones: 15-next, 15.3.0 Jul 13, 2016
zpao pushed a commit that referenced this pull request Jul 13, 2016
* Warn if PropType function is called in production

* Check if console is undefined before warning

* Randomize value of ReactPropTypesSecret

* Remove dev environment tests

* Rename typeCheckPass to productionWarningCheck

* Rename productionWarningCheck to expectWarningInProduction

* Call toString on Math.random()

* Rename test block for React type checks

* Make sure warning isnt emitted for failing props

* Cache warning by component and prop, warn in dev

* Pass ReactPropTypesSecret to internal checks

* Move tests to ReactPropTypes-test.js

* Update the warning message to include link

* Do not test warning for unions  with invalid args

(cherry picked from commit 95ac239)
@jedwards1211
Copy link
Contributor

@spicyj but there were plenty of other great possibilities! like SECRET_DO_NOT_PASS_THIS_EXPLICITLY_OR_YOU_WILL_BE_DEFENESTRATED. I always wish there were more opportunities for jokes embedded in code.

gaearon added a commit to gaearon/react that referenced this pull request Oct 23, 2016
gaearon added a commit to gaearon/react that referenced this pull request Oct 24, 2016
gaearon added a commit that referenced this pull request Oct 24, 2016
* Strip PropTypes in production build

* Revert "Warn if PropType function is called manually (#7132)"

This reverts commit e75e8dc.
gaearon added a commit to gaearon/react that referenced this pull request 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 pull request 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 pull request 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
acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017
* Strip PropTypes in production build

* Revert "Warn if PropType function is called manually (facebook#7132)"

This reverts commit e75e8dc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants