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

take care of hermes javascript engine #78

Closed
wants to merge 1 commit into from
Closed

Conversation

ice6
Copy link

@ice6 ice6 commented Nov 4, 2021

It is sad that hermes do not support regex named group.

https://github.com/facebook/hermes/blob/main/doc/RegExp.md

same issue others encountered as me :)

facebook/hermes#89

because this library is so foundamental and small, it will be a pleasure if this library can do the favor.

:)

Copy link
Member

@Qix- Qix- left a comment

Choose a reason for hiding this comment

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

I don't normally care to support non-compliant javascript engines but in this case the change makes sense. Named groups and destructuring is unnecessary here and only burns cycles anyway.

Just one change from me.

@@ -129,12 +129,12 @@ function assembleStyles() {
},
hexToRgb: {
value: hex => {
const matches = /(?<colorString>[a-f\d]{6}|[a-f\d]{3})/i.exec(hex.toString(16));
const matches = /([a-f\d]{6}|[a-f\d]{3})/i.exec(hex.toString(16));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const matches = /([a-f\d]{6}|[a-f\d]{3})/i.exec(hex.toString(16));
const matches = /[a-f\d]{6}|[a-f\d]{3}/i.exec(hex.toString(16));

@sindresorhus
Copy link
Member

While this is a seemingly simple change, it's a slippery slope. Hermes only supports ES6, so this issue will be hit again in the future.

@ice6
Copy link
Author

ice6 commented Nov 5, 2021

While this is a seemingly simple change, it's a slippery slope. Hermes only supports ES6, so this issue will be hit again in the future.

It will be ok to me if you add me as a maintainer of this little library. :)

@ice6
Copy link
Author

ice6 commented Nov 7, 2021

for those suffering such little problem, now I recommend https://www.npmjs.com/package/patch-package.
as the package claimed, it is indeed a savior.

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