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

checkPropTypes: add argument that allows external logging #54

Open
wants to merge 1 commit into
base: master
from

Conversation

@rufman
Copy link

commented May 17, 2017

When using prop-types for checking things outside of React, having checkPropTypes throw an error instead of swallowing it and logging it to console.error can be useful. The flag throwErrors will throw the error if set to true instead of logging it. AcheckPropTypeWithErrors convenience function, which calls checkPropTypes with throwErrors set to true has also been added.

Fixes #34

@facebook-github-bot

This comment has been minimized.

Copy link

commented May 17, 2017

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot

This comment has been minimized.

Copy link

commented May 17, 2017

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@rufman rufman force-pushed the rufman:master branch from 4ad04bf to 92c745e May 18, 2017

@rufman

This comment has been minimized.

Copy link
Author

commented Jun 12, 2017

@gaearon Is this a good approach to using prop-types checking outside of React? Or is there a better way?

@gaearon

This comment has been minimized.

Copy link
Member

commented Jun 12, 2017

Sorry, we're busy with many other things right now, and it's unlikely we'll have time to review PRs to prop-types very soon. In this PR, a few things concern me:

  • Creating errors is expensive and we'd like to get away from that, but throwing reinforces this pattern
  • This doesn't expose other important information (such as component stack)

@rufman rufman force-pushed the rufman:master branch from 92c745e to 2b39bb1 Jul 13, 2017

@rufman

This comment has been minimized.

Copy link
Author

commented Jul 13, 2017

The PR has been updated to not throw an error, but instead calls the function passed in as an argument, with the same arguments as are passed to warning.

@rufman rufman changed the title checkPropTypes: add flag to throw the error instead of logging checkPropTypes: add argument that allows external logging Aug 16, 2017

@rufman rufman force-pushed the rufman:master branch from 2b39bb1 to 5351558 Sep 5, 2017

@rufman

This comment has been minimized.

Copy link
Author

commented Sep 5, 2017

I'm aware that there might not be time to review this PR, but with React 16 in beta now the resolution on how to use prop-types with custom errors is becoming a more prominent issue.

I was wondering if there is anything else that would need to be done on this PR to have it merged. Or if such a feature won't be considered for the prop-types package, we'll look into alternatives.

@kaiyoma

This comment has been minimized.

Copy link

commented Sep 28, 2017

Any traction here? This is something we're interested in as well.

@martinhorsky

This comment has been minimized.

Copy link

commented Sep 29, 2017

@kaiyoma I used check-prop-types module mentioned here: #28 (comment)

ojab added a commit to ojab/redux-orm-proptypes that referenced this pull request Oct 3, 2017
Switch to prop-types & PropTypes.checkPropTypes
PropTypes.checkPropTypes cannot throw yet (see [1]), so checking
console.error output

[1] facebook/prop-types#54
ojab added a commit to ojab/redux-orm-proptypes that referenced this pull request Oct 3, 2017
Switch to prop-types & PropTypes.checkPropTypes
PropTypes.checkPropTypes cannot throw yet (see [1]), so checking
console.error output

[1] facebook/prop-types#54
ojab added a commit to ojab/redux-orm-proptypes that referenced this pull request Oct 3, 2017
Switch to prop-types & PropTypes.checkPropTypes
PropTypes.checkPropTypes cannot throw yet (see [1]), so checking
console.error output

[1] facebook/prop-types#54
ojab added a commit to ojab/redux-orm-proptypes that referenced this pull request Oct 3, 2017
Switch to prop-types & PropTypes.checkPropTypes
PropTypes.checkPropTypes cannot throw yet (see [1]), so checking
console.error output

[1] facebook/prop-types#54
ojab added a commit to ojab/redux-orm-proptypes that referenced this pull request Oct 3, 2017
Switch to prop-types & PropTypes.checkPropTypes
PropTypes.checkPropTypes cannot throw yet (see [1]), so checking
console.error output

[1] facebook/prop-types#54
ojab added a commit to ojab/redux-orm-proptypes that referenced this pull request Oct 3, 2017
Switch to prop-types & PropTypes.checkPropTypes
PropTypes.checkPropTypes cannot throw yet (see [1]), so checking
console.error output

[1] facebook/prop-types#54
ojab added a commit to ojab/redux-orm-proptypes that referenced this pull request Oct 3, 2017
Switch to prop-types & PropTypes.checkPropTypes
PropTypes.checkPropTypes cannot throw yet (see [1]), so checking
console.error output

[1] facebook/prop-types#54
ojab added a commit to ojab/redux-orm-proptypes that referenced this pull request Oct 3, 2017
Switch to prop-types & PropTypes.checkPropTypes
PropTypes.checkPropTypes cannot throw yet (see [1]), so checking
console.error output

[1] facebook/prop-types#54

@rufman rufman force-pushed the rufman:master branch from 5351558 to 0263e43 Oct 4, 2017

@JoeDuncko

This comment has been minimized.

Copy link

commented Jun 29, 2018

This would also be super useful to me. Any word on whether this might make it in? If it won't, then closing this PR and "officially" referring users to another project that does this might be a good move.

@kushthedude
Copy link

left a comment

Remove Conflicts

@ljharb
Copy link
Collaborator

left a comment

This needs a rebase.

checkPropTypes.js Outdated Show resolved Hide resolved

@rufman rufman force-pushed the rufman:master branch from a358ffa to 974b55f Sep 4, 2019

checkPropTypes.js Outdated Show resolved Hide resolved
checkPropTypes: add argument that allows external logging
When specified, the argument `warningLogger` will be called with the
same arguments as `warning`. Instead of logging errors to the fbjs
warning logger, we are able to handle them externally.

Fixes #34

@rufman rufman force-pushed the rufman:master branch from 974b55f to bfe8ccd Sep 4, 2019

@ljharb
ljharb approved these changes Sep 4, 2019
Copy link
Collaborator

left a comment

This looks good, but I'm going to hold off merging it for a bit; while adding an argument is generally semver-minor, if anyone was passing a non-function here, this would suddenly throw (and i think throwing when they pass a truthy non-function is the right behavior)

@ljharb ljharb self-assigned this Sep 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.