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

feat: introduce forbidUnknowProps #46

Closed
wants to merge 2 commits into from

Conversation

HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Sep 23, 2019

I introduced this component because propTypes.exact does something slightly different than we needed....

For example this is how propTypes.exact is used:

MyComponent.propTypes = {
    house: PropTypes.exact({
        streetName: PropTypes.string,
        houseNumber: PropTypes.number,
    }),
    otherProp: propTypes.bool,
}

And this would be a similar example of how this new function forbidUnknowProps can be used:

MyComponent.propTypes = forbidUnknowProps({
    houseNumber: propTypes.number,
    streetName: propTypes.string,
    otherProps: propTypes.bool,
})

So with forbidUnknowProps we can wrap an entire prop-type definition and with exact we operate at property level.

As another note, normally we would export two "flavours": 1) the normal function and 2) the isRequired function. In this case that didn't make much sense....

@varl
Copy link
Contributor

varl commented Sep 23, 2019

There's a typo "forbidUnknowProps" (should be "forbidUnknownProps") in the documentation and code. Mind cleaning that up?

@varl
Copy link
Contributor

varl commented Sep 23, 2019

I think that whitelistProps (or secondarily allowedProps) is a more succinct name than forbidUnknownProps.

I don't like having a negative in the function name as it raises some questions about the parameters passed in.

Copy link
Contributor

@varl varl left a comment

Choose a reason for hiding this comment

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

Spelling errors and a comment about the chosen name of the function.

@ismay
Copy link
Contributor

ismay commented Sep 23, 2019

Maybe we should wait for this issue to be resolved (or help out): facebook/prop-types#263. It seems our use-case should be supported by exact from the prop-types package.

I would prefer that over this solution. It's more standard, and others will understand easier what's going on.

@HendrikThePendric
Copy link
Contributor Author

@ismay that issue has been open since March, you think it would be worth the wait?

@HendrikThePendric
Copy link
Contributor Author

@varl function name has been corrected to whitelistProps as per your suggestion

@ismay
Copy link
Contributor

ismay commented Sep 23, 2019

that issue has been open since March, you think it would be worth the wait?

I don't know what the holdup is. Depends if it'll need a ton more work or if it's nearly there. If it's close I'd prefer using the prop-types package over building and maintaining it ourself. That'll be easier for us and more clear for outside/new devs.

@varl
Copy link
Contributor

varl commented Sep 23, 2019

The hold-up seems to be that propTypes.exact is not meant to work like that according to the docs. Some consider it a bug. Others do not. One of the maintainers has implemented this in a separate library: https://github.com/airbnb/prop-types-exact so seems to me like this is by design?

@ismay
Copy link
Contributor

ismay commented Sep 23, 2019

One of the maintainers has implemented this in a separate library: https://github.com/airbnb/prop-types-exact so seems to me like this is by design?

That dude is fairly direct with closing PRs that are wontfix or out of scope somehow though. I'd expect him to have closed it if it wouldn't fit. But I don't know. We can ask, see what the hold up is, and why he created a separate lib implementing exactly that.

@HendrikThePendric
Copy link
Contributor Author

HendrikThePendric commented Sep 24, 2019

Closing this PR for the following reasons:

@varl varl deleted the feat/forbidUnknownProps branch October 28, 2020 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants