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

Add support for object spread/rest operator. #128

Merged
merged 1 commit into from
Apr 9, 2018

Conversation

2Pacalypse-
Copy link
Contributor

Before this commit, using rewire on a module that had object spread/rest
operator in it would throw an exception. Now it doesn't.

Before this commit, using rewire on a module that had object spread/rest
operator in it would throw an exception. Now it doesn't.
@coveralls
Copy link

coveralls commented Jan 24, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 122689e on 2Pacalypse-:master into 18c5d0e on jhnns:master.

@pdworniczak
Copy link

Will that pull request be merged anytime soon?

@rbriank
Copy link

rbriank commented Feb 22, 2018

When will this be merged into master?

@dimitriirybakov
Copy link

That's very important feature, because nodejs supports object spread. Merge it, please.

@okv
Copy link

okv commented Mar 9, 2018

Hi there.
@jhnns any chance that pull request will be merged?)

@jhnns jhnns merged commit d9a81c0 into jhnns:master Apr 9, 2018
@okv
Copy link

okv commented Apr 9, 2018

That's great, @jhnns could you also publish it to npm?

jhnns added a commit that referenced this pull request Apr 9, 2018
This regex replacement is not 100% safe because transforming JavaScript requires an actual parser.
However, parsing (e.g. via babel) comes with its own problems because now the parser needs to
be aware of syntax extensions which might not be supported by the parser, but the underlying
JavaScript engine. In fact, rewire used to have babel in place but required an extra
transform for the object spread operator (check out commit d9a81c0
or see #128). It was also notable slower
(see #132).
There is another issue: replacing const with let is not safe because of their different behavior.
That's why we also have ESLint in place which tries to identify this error case.
There is one edge case though: when a new syntax is used *and* a const re-assignment happens,
rewire would compile happily in this situation but the actual code wouldn't work.
However, since most projects have a seperate linting step which catches these const re-assignment
errors anyway, it's probably still a reasonable trade-off.

Fixes #132
@jhnns
Copy link
Owner

jhnns commented Apr 9, 2018

Thanks for this PR 👍 Although I've refactored it (removed babel again), it was still an important commit :)
Published as 4.0.0

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.

7 participants