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

Investigate using regexpp Instead of regexp-to-ast #777

Closed
bd82 opened this issue Jul 12, 2018 · 15 comments
Closed

Investigate using regexpp Instead of regexp-to-ast #777

bd82 opened this issue Jul 12, 2018 · 15 comments

Comments

@bd82
Copy link
Member

bd82 commented Jul 12, 2018

  • Is it more complete (e.g. unicode flag support).
  • Performance?
  • Runs in the browser / IE ?
  • Which versions of node.js are supported?
@bd82
Copy link
Member Author

bd82 commented Apr 19, 2019

I am not sure migration would be worth the effort.
It does not seem like maintaining regexp-to-ast requires too much effort.

@bd82 bd82 closed this as completed Apr 19, 2019
@bd82
Copy link
Member Author

bd82 commented Aug 30, 2019

Now that several new regExp features need to be supported (lookbehind/named capture groups)
It may be worth re-evaluating this.

It could also be evaluated in terms of performance easily as there is a (minimal) cold start benchmark now.

@bd82
Copy link
Member Author

bd82 commented Apr 18, 2020

Had another quick look, This seems even more interesting to examine as:

  • regexpp is still actively maintained.
  • regexpp has no deps
  • NodeJS V8.0.0 is supported
  • It is 2020, perhaps it is time to ditch IE11 support instead of worrying about compatibility...

@bd82
Copy link
Member Author

bd82 commented May 29, 2020

Performance of regexpp seems very similar to regexp-to-ast

regexp-to-ast Benchmark
Current version x 10,999 ops/sec ±0.77% (91 runs sampled)
regexpp x 11,116 ops/sec ±0.50% (93 runs sampled)
Fastest is regexpp,Current version

@bd82
Copy link
Member Author

bd82 commented Feb 28, 2021

The regexpp does not seem to be well maintained at this time, perhaps the plans for swapping it out should be shelved.

@mattbishop
Copy link

Actually it seems pretty active compared to regexp-to-ast. Latest release June 14, 2021 vs Dec 19, 2019.

Also, and crucially, regexpp supports unicode escape sequences and groups. This is a must-have here in 2021!

@bd82
Copy link
Member Author

bd82 commented Oct 7, 2021

Hi @mattbishop

I tried to recall what were my previous issue with regexpp
and rediscovered this previous blocker issue I opened more than a year ago.

I'd still prefer to use a 3rd party library, but this means I need to find a way to patch it.

@bd82
Copy link
Member Author

bd82 commented Oct 7, 2021

It may be possible to combine https://www.npmjs.com/package/patch-package with "bundledDependencies" to resolve (patch-hack) regexpp

@mattbishop
Copy link

mattbishop commented Oct 7, 2021 via email

@bd82
Copy link
Member Author

bd82 commented Oct 7, 2021

Would it be hard to create a PR? I am assuming you have already forked

I created one over a year ago...

That is why I need some hack to apply the patch as a consumer 😄

@bd82
Copy link
Member Author

bd82 commented Oct 7, 2021

Assuming an hack is possible, the bigger work would the integration of regexpp instead of regexp-to-ast
I started this a while ago, to be honest I don't recall the status it is in

@mattbishop
Copy link

mattbishop commented Oct 8, 2021 via email

@bd82
Copy link
Member Author

bd82 commented Oct 14, 2021

I am a little conflicted to be honest.

On one hand it would be very nice to have a blackbox providing all the regexp parsing needs.
On the other hand:

  • regexpp does not actually provide all the needed APIs
  • It does not seem to be actively maintained.

Maybe I should instead focus on updating regexp-to-ast to most modern regexp syntax.
and maybe even make it part of this monorepo to reduce maintenance overhead.
It is quite possible that updating regexp-to-ast to modern ECMAScript spec would require similar effort to refactoring Chevrotain to use regexpp...

@mattbishop
Copy link

Well it is nicer to have control of such an important lib. If you move it into the monorepo then it can stay in sync easier. Is anyone outside of chevrotain using it?

@bd82
Copy link
Member Author

bd82 commented Oct 15, 2021

There are a handful of dependents.

But they can still consume it if I move it to @chevrotain/regexp-to-ast package.
At the cost of losing meaningful semantic versioning numbers.

anyhow I am closing this in favor of #1670
When I get around to attempting to modernize the regexp support
I'll re-evaluate the current status and make a decision.

@bd82 bd82 closed this as completed Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants