Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

add rule no danger props and tests #428

Merged
merged 1 commit into from
May 16, 2020
Merged

Conversation

VictorHom
Copy link
Contributor

The rule is from here

It's based on the list in #341

Also the credit pretty much goes to @bitpshr in his pr #421

@VictorHom
Copy link
Contributor Author

@sebmck not sure what the issue is with ci error. can you help me when you have a chance?

@bitpshr
Copy link
Contributor

bitpshr commented May 14, 2020

I think this rule should be considered carefully before it's landed. It's not a recommended rule, it's not used by create-react-app, and the less-restrictive react/no-danger-with-children rule is already implemented. If this rule does end up landing, react/no-danger-with-children should in turn be removed to prevent duplicate warnings when children is also set.

@VictorHom
Copy link
Contributor Author

@bitpshr good point. not familiar with what's used in react-create-app, but I agree with that the duplicated messaging wouldn't be good

@sebmck
Copy link
Contributor

sebmck commented May 16, 2020

This looks reasonable. We are going with the strategy of "over the top" lint rules with suppressions preferred. This is one of the things that is wrong 99% of the time. What do you think about the rule overlap?

@bitpshr
Copy link
Contributor

bitpshr commented May 16, 2020

Thinking about this more, rule overlap isn't a bad thing and will happen in other cases, too. For example, duplicate object keys will be checked in addition to duplicate JSX props, even though they technically overlap in the case of createElement. Throwing different (or tangentially-related) errors for the same lines won't hurt anything.

@sebmck
Copy link
Contributor

sebmck commented May 16, 2020

Sounds good to me. We can fine tune our strategy in the future if it turns out to be excessive.

@sebmck sebmck merged commit 28ac40a into rome:master May 16, 2020
@sebmck
Copy link
Contributor

sebmck commented May 16, 2020

Thanks all!

@VictorHom VictorHom deleted the noDanger branch May 17, 2020 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants