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
Conversation
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 |
It may be that node 4's regexes don't have the proper native unicode support, which is why xregexp is needed. |
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. As for the CI issue, it doesn't seem to be RegExp related; there is a syntax error on an innocuous expression. |
There was a problem hiding this 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.
9c4467a
to
4f038ed
Compare
Your suggestion of removing the caching of |
Thank you very much for the prompt review by the way! 🤗 |
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. |
What is needed to make this PR mergeable? |
@ljharb Polite bump 🤗
|
FWIW this also removes a dependency on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming tests pass.
Reimplemented the logic of extended regular expressions with functions in the
jsx-pascal-case
rule in order to remove the dependency onxregexp
. This reduced the size ofnode_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:(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.