Remove named capture group

[danilobuerger]

Follow up to #78 with the requested change. Also fixes #76

It makes sense not wanting to add workarounds for random engines (like hermes). However, I would argue that this is not a workaround. Instead it makes the code easier to read and more performant. There is no real reason to use named capture groups here.

[bombillazo]

This is great! This seems to be breaking my React Native build when I updated one of my dependencies.

[Karstagg]

This issue is also breaking my React Native build due to an updated dependency. Applying this fix via a patch works ~ thanks! 👍

[Qix-]

#78 (comment)

We've covered this before and have closed pretty much this exact PR.

Is there a reason React Native doesn't have modern Javascript support? Has an issue been opened with them? Named regex groups are everywhere - it's very strange that you're having issues. Are you running the most recent version?

If we fix it here, you're going to hit it again in the future. Personally I'm not opposed to these changes as we're not actually using the groups for anything novel or specific and they do have a slight (very, very slight, like almost immeasurable) overhead.

But changing it here isn't going to solve your problems with React Native or whatever other javascript engine forever. It's just a matter of time before someone else adds groups to their regex patterns and you're right back at square one.

I'll once again defer to @sindresorhus here (sorry to put more on your plate) but I would highly suggest either upgrading or opening an issue with Facebook about their lack of support of modern Javascript...

---

Side note to the author of the comment above that was hidden: Do not come to Github and make demands. It won't get you very far. Be a good citizen of open source, please. We do this on our free time, and this package is used by millions of projects. We don't just do things "ASAP" because it takes up our time, too.

[danilobuerger]

Hi @Qix- 👋 , thanks for taking the time. Based of your comment in #78 I made the PR:

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.

I didn't get the impression from the last PR that this was not wanted. It seemed to me that the PR was just closed by the Author because he worked around it.

Is there a reason React Native doesn't have modern Javascript support? Has an issue been opened with them? Named regex groups are everywhere - it's very strange that you're having issues. Are you running the most recent version?

Hermes (not React Native) does support only ES6 by design. Named Groups are ES2018 afaik. The issue is at facebook/hermes#89 .

But changing it here isn't going to solve your problems with React Native or whatever other javascript engine forever. It's just a matter of time before someone else adds groups to their regex patterns and you're right back at square one.

I understand that argument. On the other side: usually React Native projects pull in a huge amount of dependencies and this is the only one that fails with Hermes, out of a thousand.

My guess is when React Native 0.68 gets released (its currently in rc), a lot more people will come here and complain. It's a simple change here, that comes with minor benefits and no real downside. I understand if you don't want to merge this out of principle, but it would unblock a lot of people (as you can already see by the amount of people finding this PR which I haven't advertised anywhere).

No matter the outcome here, I will try to raise this issue again with the Hermes team to see if we can get them to reconsider their position on named groups.

[Karstagg]

Just as some background here - I think people might be hitting this issue sooner than RN 0.68, I'm using 0.67 at the moment. ansi-styles is a dependency of several packages / dependencies of those packages. Most are still using a version lower than 5 (probably for this reason), but pretty-format uses >5.

I can understand not wanting to go back in time for the sake of one JS engine, but if this change can't be merged maybe there could be a note in the readme or an open issue to point people in the right direction?

[Cordobo]

EDIT: See @danilobuerger comment below, has been fixed for 0.69+

For those looking for a workaround, adding "resolutions" (yarn) and "overrides" (npm) for ansi-styles to package.json helped us in a React Native 0.68.3 app with Hermes support enabled (both iOS and android).

  "resolutions": {
    "ansi-styles": "4.3.0"
  },
  "overrides": {
    "ansi-styles": "4.3.0"
  },

HTH till we get some ES 2018 love in Hermes

[danilobuerger]

This was fixed in react-native starting with v0.69.0 (facebook/metro#791, metro release 0.70.1). So this PR is no longer necessary for react-native.

I still consider this PR valid as named groups are just not necessary, raising the es level just because of some novel feature makes little sense. Maybe now that react-native is out of the picture, you could consider it on its other merits @Qix- ?

[Qix-]

I'm in favor. I'll defer to @sindresorhus if he has any strong opinions.

[sindresorhus]

I still consider this PR valid as named groups are just not necessary, raising the es level just because of some novel feature makes little sense.

I agree that a named group is not actually needed here.