-
Notifications
You must be signed in to change notification settings - Fork 47.1k
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
Use hardcoded value for PropType secret #7194
Conversation
If I were third party lib author I wouldn’t feel guilty about passing a string like this. |
|
Whichever. |
There are already firing warnings elsewhere in the code, and I think it would make for a good running joke in the codebase 😄 |
Rename secret!
eabf6a7
to
40f63c0
Compare
I went with this one, for consistency 😉 |
→ semver-minor since it depends on another semver-minor (good edge case testing for my tool, but going to be hard to make work well when trying to do patch-only releases) |
Rename secret!
Rename secret! (cherry picked from commit 2c93a41)
Our tests fail on console warnings, so sadly this causes a semver-minor React update to break things :-/ Also, https://facebook.github.io/react/warnings/dont-call-proptypes.html 404s so I'm not even sure why it's a problem. |
This puts us in a very difficult position because this effectively means we can’t introduce new deprecations without doing a major release. Unfortunately this would slow down our development way too much, and for the benefit of the community we can’t do it this way. When we introduced our new versioning strategy with 15.0.0, we clearly said that we ship deprecation warnings in minor releases. This does not technically contradict semver. The decision to fail tests on console warnings is yours. I can see why this is reasonable, but in this case you’ll want to use
That’s bad, probably CI is stuck. I’ll look into why this happened, thanks for flagging. |
@gaearon Understood. However, our tests aren't run in production mode nor run with a production build of react, so I'm not sure why they'd be issuing warnings, since the point of the original issue was to deprecate calling them directly in production. We're still looking into it, and I'll update when I have more info. |
Many people won’t notice warnings that happen only in production builds. Our intention is to deprecate the pattern of calling them directly per se because we want to treat them more as metadata. So we want the libraries that use |
We use them to build custom propType validators on top of them - that are also stripped in production. What should we use instead if we want identical behavior as React's native PropTypes? |
This is still supported. (The link is up now.) |
Thanks, that will likely resolve our problem :-) |
I know this is a long standing hard coded value, but is there a way we can change the value to exclude the word "Secret"? Our security scans are picking this up as a hard coded password/secret due to the value and naming. Unfortunately we cannot change the config of the security scans or flag this as a false positive as the config of the security scanning software is defined by the client. |
The client should change their config to not pick up false positives, or the software shouldn't be scanning third-party code for secrets ¯\_(ツ)_/¯ |
Addresses @spicyj's concern #7132 (comment)
cc @gaearon