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

Export resetWarningCache #178

Merged
merged 1 commit into from Feb 8, 2019
Merged

Export resetWarningCache #178

merged 1 commit into from Feb 8, 2019

Conversation

patsplat
Copy link

@patsplat patsplat commented Apr 8, 2018

When testing code using PropTypes, warnings fail unexpectedly due to caching behavior. It would be useful for resetWarningCache to be part of the publicly exported API.

  • Added test for current caching behavior of PropTypes.checkPropTypes
  • Added test for new reset function PropTypes.checkPropTypes.resetWarningCache
  • Implemented PropTypes.checkPropTypes.resetWarningCache
  • Documented PropTypes.checkPropTypes.resetWarningCache

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This is excellent; in our tests we never want to suppress warnings, so we’d run this before every test.

@patsplat
Copy link
Author

patsplat commented Apr 9, 2018

Thanks for the 👍

This addition is also intended to make spying console.error for expected warnings reliable. Currently mocha --watch fails on rebuilds.

@ljharb
Copy link
Collaborator

ljharb commented Apr 9, 2018

cc @aweary

@patsplat
Copy link
Author

Hi! Any updates or feedback on this PR?

@patsplat
Copy link
Author

Closing due to inactivity. If you are interested in this feel free to reopen.

@patsplat patsplat closed this Apr 30, 2018
@ljharb
Copy link
Collaborator

ljharb commented May 1, 2018

Please reopen this; inactivity is irrelevant.

@patsplat patsplat reopened this May 1, 2018
@patsplat
Copy link
Author

patsplat commented Jun 4, 2018

@ljharb any suggestions on next steps for this PR?

@ljharb
Copy link
Collaborator

ljharb commented Jun 4, 2018

Unfortunately we have to wait for the maintainers to take a look.

@gaearon
Copy link
Contributor

gaearon commented Jun 19, 2018

Is there any specific reason it's on checkPropTypes, and not on the PropTypes object itself?

I'm worried if attaching things to functions can mess with uglify/rollup "pure" code detection. Not that it matters for this piece of code but still.

@ljharb
Copy link
Collaborator

ljharb commented Jun 19, 2018

It does seem useful to me to have it tucked away somewhere - devs are less likely to casually use it that way.

@patsplat
Copy link
Author

Implemented on checkPropTypes as that is the unit whose state is modified.

@patsplat
Copy link
Author

Happy if library maintainers want to move the interface around, but figured it could start in the most naive spot first.

@uniphil
Copy link

uniphil commented Jun 19, 2018

This would be awesome! It would make eg. https://github.com/ratehub/check-prop-types obsolete, which would make me (its creator) very happy.

@patsplat
Copy link
Author

@gaearon changed the public interface to PropTypes.resetWarningCache. Still need to reach inside checkPropTypes to clear the cache.

Are there any other recommended changes?

@uniphil
Copy link

uniphil commented Feb 8, 2019

💃

@patsplat
Copy link
Author

patsplat commented Mar 1, 2019

Thanks @ljharb !

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.

None yet

5 participants