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

Fixes #6153 Add case typechecking to provided props against propTypes #6938

Closed
wants to merge 3 commits into from

Conversation

brad-decker
Copy link

Grabs mismatched values compared to PropTypes, can be used later to warn against props being provided that aren't defined in PropTypes.

It then uses toLowerCase to find similar props and warn if there is a defined propType with the same composition but difference case as a prop passed on the element.

Warning follows the "did you mean %s?" type suggestions in other react warnings.

reactEurope-hackathon

@ghost
Copy link

ghost commented Jun 1, 2016

@brad-decker updated the pull request.

@gaearon
Copy link
Collaborator

gaearon commented Jun 26, 2016

This seems to achieve the same goal as #6255. It doesn’t seem to dedupe warnings though. In general, I’m not sure if we want this. See #6255 (comment) for some comments. Let’s first reach a decision on that, and then work on PRs.

@brad-decker
Copy link
Author

@gaearon Thanks for the response. I'm fine with waiting for a decision on this and closing this PR if there is a better solution or one more inline with the direction you want to go.

@gaearon
Copy link
Collaborator

gaearon commented Jun 27, 2016

Can you please make this deduplicate warnings? I would probably go with this rather than with a full-blown devtool approach in this particular case. But we need to ensure we don’t spam the console with warnings about the same prop multiple times.

@brad-decker
Copy link
Author

@gaearon definitely. Will work on this shortly.

@aweary
Copy link
Contributor

aweary commented Jan 26, 2017

@brad-decker any chance you could get the warning deduplicated? I closed #6255 so this PR is next in line if you want to continue working on it 😃

@aweary aweary self-assigned this Jan 26, 2017
@brad-decker
Copy link
Author

@aweary Yep! will get this done today. Thanks for the nudge ;) :D

@brad-decker
Copy link
Author

@aweary added warning depulication, rebased and squashed down to two commits. First of which is the work from React-Europe 7 months ago then the new work in the new commit. Can squash and reword these to something that is more inline with what the React team would like for commit messages once reviewed.

@brad-decker
Copy link
Author

Any assistance with the failed tests and a point in the right direction for debugging my impact on them would be welcomed.

@aweary
Copy link
Contributor

aweary commented Jan 26, 2017

Thanks @brad-decker! I'll come back to this later today to review again. I think there's a way we can avoid a couple of the indexOf checks to make this a bit faster.

Any assistance with the failed tests and a point in the right direction for debugging my impact on them would be welcomed.

Go ahead and run the ./scripts/fiber/record-tests script. We use it to track which unit tests are failing/passing for Fiber and if its not run when a new test is added CI will fail.

@brad-decker
Copy link
Author

@aweary got it. thanks

@brad-decker
Copy link
Author

@aweary, If this isn't planned to land soon thats cool. I'd like to still get your input and feedback on any improvements you think could be made to this when you have time to review. Thanks again!

@aweary
Copy link
Contributor

aweary commented Apr 9, 2017

PropTypes have been removed from React core and now exist as a separate package prop-types. Any future feature requests, bug reports, or changes should be directed to the new repo at https://github.com/reactjs/prop-types.

Sorry about that @brad-decker!

@aweary aweary closed this Apr 9, 2017
@aweary
Copy link
Contributor

aweary commented Apr 9, 2017

If you'd like to continue working on this I've opened a corresponding issue at facebook/prop-types#7

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.

4 participants