Skip to content

Use more efficient regex for boolean-prop-naming #2190

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jordanbtucker
Copy link

The default RegExp for the boolean-prop-naming rule (^(is|has)[A-Z]([A-Za-z0-9]?)+) has an odd syntax that is inefficient and does not test the entire property name.

For example, the following propTypes passes with the current RegExp.

var Hello = createReactClass({
  propTypes: {
    // This property name has an underscore, but still
    // passes as a valid boolean property name.
    isGood_Enough: PropTypes.bool
  },
  render: function() { return <div />; };
});

This PR assigns a new default RegExp (^(is|has)[A-Z][A-Za-z0-9]*$) that is not only more performant but also tests the entire property name.

@jordanbtucker
Copy link
Author

This should probably be rebased with or merged into #1702.

@ljharb
Copy link
Member

ljharb commented Mar 6, 2019

This would be a breaking change; isGood_Enough is likely intentionally allowed. Separately, performance rarely matters, and the performance of a regex applied in linting - not even a hot code path - also doesn't matter.

If you can improve the regex while keeping what it matches identical, I'd love to merge that!

@jordanbtucker jordanbtucker changed the title Use more accurate and performant regex for boolean-prop-naming Use more efficient regex for boolean-prop-naming Mar 6, 2019
@jordanbtucker
Copy link
Author

jordanbtucker commented Mar 6, 2019

I understand if you don't want to introduce a breaking change. An even more efficient yet compatible RegExp is ^(is|has)[A-Z]. I've updated the PR to reflect that. Let me know if you want me to squash the commits.

I still disagree that isGood_Enough is intentionally allowed. The original RegExp appears to have meant to check the entire property name and it did not include any underscores. Note that the property name isA_$$$_Maker is also valid with the original RegExp. It seems odd to enforce a camelCase prefix and then allow snake_case for the rest of the property name.

One issue I still see with this improvement is that it makes it less clear what is intended. For example a user might see the lint error Prop name (enabled) doesn't match rule (^(is|has)[A-Z]) and conclude that boolean property names are only allowed to have one uppercase letter after is or has and nothing else. Alternatively, I can set the default RegExp to ^(is|has)[A-Z][A-Za-z0-9]*. This is less efficient than ^(is|has)[A-Z] but is still more efficient than the original and it is easier for humans to understand. However, this can also be misleading since users may think that it checks the entire property name.

@ljharb ljharb added the semver-major Breaking change. label Apr 12, 2019
@ljharb ljharb force-pushed the master branch 6 times, most recently from 59af733 to 865ed16 Compare November 11, 2022 02:45
@ljharb ljharb force-pushed the master branch 4 times, most recently from 069314a to 181c68f Compare November 18, 2022 17:19
@ljharb ljharb force-pushed the master branch 2 times, most recently from 380e32c to 51d342b Compare July 4, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Breaking change.
Development

Successfully merging this pull request may close these issues.

2 participants