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

Support regex named capture groups #696

Closed
bombillazo opened this issue Mar 11, 2022 · 12 comments
Closed

Support regex named capture groups #696

bombillazo opened this issue Mar 11, 2022 · 12 comments
Labels
enhancement New feature or request

Comments

@bombillazo
Copy link

bombillazo commented Mar 11, 2022

Problem

Follow up on #89 :
Currently, when using Hermes to build a React Native app, when it tries to include a dependency using regex named capture groups, the app build process crashes.

Example code from real dependency:

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

Build Error: SyntaxError: Invalid RegExp pattern: Quantifier has nothing to repeat, js engine: hermes
'?<colorString>' being the invalid pattern.

Solution

I am unfamiliar with the required work to solve this, but I am assuming the Hermes Javascript engine core would need to be upgraded to add support for named capture groups. Update the engine parse/lexer to process named groups? Use an existing regex library?

@bombillazo bombillazo added the enhancement New feature or request label Mar 11, 2022
@neildhar
Copy link
Contributor

neildhar commented Mar 11, 2022

Thanks for sharing this. As shared in the previous issue, named capture groups are an ES2018 feature that we have not implemented in Hermes yet. Note that for most cases, you should be able to get away with a babel transform for named capture groups, as described here.

@thomazcapra
Copy link

@neildhar thanks, after installing the @babel/plugin-transform-named-capturing-groups-regex and adding it in the plugin array in my babel.config.js file, it works now :) 🚀

jeanregisser added a commit to valora-inc/wallet that referenced this issue Jun 22, 2022
### Description

- Upgrade `@celo/utils` to fix vulnerability in `@umpirsky/country-list`: GHSA-hj79-42mx-m4gr
  - The upgraded `@celo/utils` caused a new parsing error with hermes as it [doesn't support named capture groups in RegEx](facebook/hermes#696). It was solved by adding `@babel/plugin-transform-named-capturing-groups-regex` to our babel transforms.
- Upgrade `shell-quote` to fix vulnerability: GHSA-g4rg-993r-mgx7

### Other changes

N/A

### Tested

The app + phone verification should work as expected

### How others should test

Please test the phone verification and confirm that it works as expected (there should be no change as a result of this PR)

### Related issues

Discussed on [Slack](https://valora-app.slack.com/archives/CL7BVQPHB/p1655807207073809).

### Backwards compatibility

Yes

Co-authored-by: Kathy Luo <kathyluo18@gmail.com>
Co-authored-by: Jean Regisser <jean.regisser@gmail.com>
@visitsb
Copy link

visitsb commented Sep 14, 2022

@neildhar thanks for the babel transform suggestion!

@fbmal7
Copy link
Contributor

fbmal7 commented Sep 27, 2022

Wanted to update this thread- named capture groups will be supported natively by Hermes soon! The work is already done, we are just making some minor changes and revisions now. It will most likely be landing in the next week or so!

@fbmal7 fbmal7 closed this as completed Sep 27, 2022
@fbmal7 fbmal7 reopened this Sep 27, 2022
@Nantris
Copy link

Nantris commented Nov 19, 2022

@fbmal7 any ETA? I found this is quite a problem for us - and unfortunately adding @babel/plugin-transform-named-capturing-groups-regex does not resolve it, surprisingly - React Native@0.70.5

@Nantris
Copy link

Nantris commented Jan 18, 2023

Friendly bump. @fbmal7 any news?

Edit: I'd also encourage people upvoting the comments further down to upvote the first comment, as repo maintainers can see which issues have the most thumbs-ups.

@fbmal7
Copy link
Contributor

fbmal7 commented Jan 18, 2023

@slapbox Sorry I should have updated this thread again earlier! Native support for named capture groups has been in hermes since Oct 7th, with c2d622.

RN 0.70 was first cut on Jul 15th. RN 0.71 was first cut on Nov 3rd. So, named capture groups won't be usable in RN 0.70, but should be there in 0.71.

@maximilize
Copy link

If you can't upgrade to 0.71 now, you also can try a workaround.

  1. Install @babel/plugin-transform-named-capturing-groups-regex. This should solve errors inside your own code
  2. Fixing node_modules is more tricky. Run the babel cli directly on the file with the invalid regexes. You can find all by using hermesc against a js bundle. Then use the regex from the warnings of hermesc and search for the files, for eg via grep -r '(?P<foobar' node_modules.
  3. Apply babel on this files: npx babel node_modules/foo/bar.js >/tmp/x.js && mv /tmp/x.js node_modules/foo/bar.js
  4. To save your changes, create patches of the modified modules: npx patch-package foobar

SiobhyB pushed a commit to wordpress-mobile/gutenberg-mobile that referenced this issue Feb 8, 2023
As per this issue, named capture groups won't be supported by Hermes until RN 0.71: facebook/hermes#696

With this commit, the latest version of '@babel/plugin-transform-named-capturing-groups-regex' has been installed to workaround this.
SiobhyB pushed a commit to WordPress/gutenberg that referenced this issue Feb 8, 2023
As per this issue, Hermes doesn't currently support named capturing groups: facebook/hermes#696

This commit addresses that by using the 'plugin-transform-named-capturing-groups-regex' plugin.
@fbmal7 fbmal7 closed this as completed Feb 23, 2023
@malwatte
Copy link

In my case I use create a Regex with the search text entered by the user.

const searchRegex = new RegExp(searchText, 'i');
items.filter(itm => searchRegex.test(itm));

Getting similar errors when users enters +, (, *, etc.

For the moment I'm adding extra \ and it fixes the issue

searchText= searchText
        .replace(/\\/gm, '\\\\')
        .replace(/\./gm, '\\.')
        .replace(/\+/gm, '\\+')
        .replace(/\(/gm, '\\(')
        .replace(/\)/gm, '\\)')
        .replace(/\?/gm, '\\?')
        .replace(/\*/gm, '\\*')
        .replace(/\|/gm, '\\|')
        .replace(/\[/gm, '\\[')
        .replace(/\]/gm, '\\]');

But I don't like this workaround. Is there a better way to fix this issue?

(I'm using react-native 0.68.3)

@ljharb
Copy link

ljharb commented Mar 20, 2023

@malwatte that's a really quick way to break things, since it's very unsafe to put unsanitized user input into a regex.

@malwatte
Copy link

@ljharb Appreciate if you can suggest better way to do. I need to do a search using the user input text. Thanks.

@Nantris
Copy link

Nantris commented Mar 21, 2023

@malwatte https://www.npmjs.com/package/escape-string-regexp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants