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

Remove dependency on xregexp #2636

Merged
merged 1 commit into from Jun 7, 2020
Merged

Conversation

yacinehmito
Copy link
Contributor

@yacinehmito yacinehmito commented May 9, 2020

Reimplemented the logic of extended regular expressions with functions in the jsx-pascal-case rule in order to remove the dependency on xregexp. This reduced the size of node_modules on my machine from 150MB to 140MB.


The dependency on xregexp was introduced in #2557 to support the full Unicode range. More specifically, it allows for using the \p{Lu} and \p{Ll} character classes, which are defined as such:

\p{Ll} or \p{Lowercase_Letter}: a lowercase letter that has an uppercase variant.
\p{Lu} or \p{Uppercase_Letter}: an uppercase letter that has a lowercase variant.

(Source)

However, this logic can be implemented in JS by the respective expressions:

  • char === char.toLowerCase() && char.toUpperCase() !== char.toLowerCase()
  • char === char.toUpperCase() && char.toUpperCase() !== char.toLowerCase()

I have used this to replace the regular expressions with simple loops.


While working on this pull request, I might have found the origin of a bug. Discussion in this issue: #1334

The code is not the most readable; I tried to strike the right balance between expressivity and performance. I also dumped everything in the rule file.
I am happy to perform any changes that would improve code quality or code organisation following the PR review.

@yacinehmito
Copy link
Contributor Author

The job is failing on the CI for Node 4 and I don't understand why. https://travis-ci.org/github/yannickcr/eslint-plugin-react/jobs/684908128
🤷

@ljharb
Copy link
Member

ljharb commented May 9, 2020

It may be that node 4's regexes don't have the proper native unicode support, which is why xregexp is needed.

@yacinehmito
Copy link
Contributor Author

yacinehmito commented May 9, 2020

Yes, and even more than that because even recent RegExp implementations don't support matching on lowercase or uppercase classes.

I didn't want to imply that xregexp was pointless; I wanted to highlight that we can replicate the functionality we get from it using simple JS methods.
It's more code to write and maintain but I believe that side of the tradeoff to be the good one.

As for the CI issue, it doesn't seem to be RegExp related; there is a syntax error on an innocuous expression.

Copy link
Member

@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.

In general, I believe that more dependencies is always better; I trust xregexp to handle edge cases more than some hand-rolled JS functions in one repo.

However, if this can replicate it and tests can pass, so be it.

lib/rules/jsx-pascal-case.js Outdated Show resolved Hide resolved
lib/rules/jsx-pascal-case.js Outdated Show resolved Hide resolved
@yacinehmito
Copy link
Contributor Author

yacinehmito commented May 9, 2020

As for the CI issue, it doesn't seem to be RegExp related; there is a syntax error on an innocuous expression.

Your suggestion of removing the caching of length solved it at the same time. It was because I was relying on destructuring, and this isn't supported in Node 4.

@yacinehmito yacinehmito requested a review from ljharb May 9, 2020 05:12
@yacinehmito
Copy link
Contributor Author

Thank you very much for the prompt review by the way! 🤗

@yacinehmito
Copy link
Contributor Author

yacinehmito commented May 9, 2020

In general, I believe that more dependencies is always better; I trust xregexp to handle edge cases more than some hand-rolled JS functions in one repo.

I share the sentiment. I looked for a package that tests casing and I sadly didn't find any. 😞

EDIT: My point is that I would also have preferred to delegate this logic to a package that knows how to identify a pascal casing. However I did not find any, and I prefer having to write and maintain 30 lines of JS over unpacking 10MB more on each fresh install.

@yacinehmito
Copy link
Contributor Author

What is needed to make this PR mergeable?

@yacinehmito
Copy link
Contributor Author

@ljharb Polite bump 🤗

What is needed to make this PR mergeable?

@dadleyy
Copy link

dadleyy commented Jun 7, 2020

FWIW this also removes a dependency on core-js which seems to be in a concerning state currently.

Copy link
Member

@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.

assuming tests pass.

@ljharb ljharb merged commit 03df672 into jsx-eslint:master Jun 7, 2020
ljharb added a commit that referenced this pull request Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants