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

Implemented transform-regexp-constructors plugin #196

Merged
merged 1 commit into from
Oct 18, 2016

Conversation

shinew
Copy link
Contributor

@shinew shinew commented Oct 17, 2016

Changes RegExp constructors to literals.

Copy link
Member

@kangax kangax left a comment

Choose a reason for hiding this comment

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

This looks great!

I just think that we might miss out on a lot of cases where strings are concatenated together — new RegExp('foo' + 'bar') or where RegExp receieves an identifer (new RegExp(RE_FOO_BAR)) or a combination of two (new RegExp(reSmth + '$')).

Can we try to account for these cases too?

@shinew
Copy link
Contributor Author

shinew commented Oct 18, 2016

Updated. Also, for RegExp constructors with 0 arguments, this would currently throw at const pattern = evals[0].value;. Catching this case at pseudo-compile time is not terrible, but should this behavior be changed/documented?

@kangax
Copy link
Member

kangax commented Oct 18, 2016

Yeah, let's definitely add a check for 0 arguments. It should transform to /(?:)/ (empty non-capturing group). Also, I would rename evals to evaluatedArgs for clarity.

@shinew
Copy link
Contributor Author

shinew commented Oct 18, 2016

Updated to gracefully handle empty RegExp constructions.

Changes RegExp constructors into literals.
@kangax kangax merged commit d54b616 into babel:master Oct 18, 2016
@kangax
Copy link
Member

kangax commented Oct 18, 2016

Thanks!

@shinew shinew deleted the transform-regexp branch October 18, 2016 22:53
@kangax
Copy link
Member

kangax commented Oct 18, 2016

@hzoo @loganfsmyth @danez do we want to also update babili preset?

@hzoo
Copy link
Member

hzoo commented Oct 18, 2016

yeah if want to add it now - we'll need the package.json for this package + adding it to the preset package.json if we do

@kangax
Copy link
Member

kangax commented Oct 18, 2016

@shinew Let's do it. That way we could also see the impact of this plugin on performance with our benchmark.

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.

4 participants