fix: include object-spread-syntax plugin #141

SimenB · 2017-12-17T08:31:40Z

Adding this plugin allows Babel to parse the syntax. It does not transpile it, so the user still needs to transpile it if running on a node version which does not support it. But this means that consumers don't have to add this plugin just so Babel can parse their code.

Will fix jestjs/jest#5082

SimenB · 2017-12-17T08:34:41Z

package.json

    "find-up": "^2.1.0",
-    "istanbul-lib-instrument": "^1.7.5",
+    "istanbul-lib-instrument": "^1.8.0",


Explicitly bumping to ensure istanbuljs/istanbuljs#82 is included

coveralls · 2017-12-17T08:36:05Z

Coverage remained the same at 96.97% when pulling 060d3de on SimenB:object-spread-syntax into 7045a03 on istanbuljs:master.

coveralls · 2017-12-17T08:38:03Z

Coverage remained the same at 96.97% when pulling f467aee on SimenB:object-spread-syntax into 7045a03 on istanbuljs:master.

coveralls · 2017-12-17T08:38:03Z

Coverage remained the same at 96.97% when pulling f467aee on SimenB:object-spread-syntax into 7045a03 on istanbuljs:master.

JaKXz

This change makes sense to me. Just want to get @bcoe's thoughts, specifically on how broad this change actually applies first before merging. i.e., would it just make more sense to make the change in jest's wrapper around babel-plugin-istanbul? Does this apply to other cases/test runners/ in the general case?

My understanding could be wrong but I don't see the harm of using this in the general case.

Thanks for the contribution :)

damianobarbati · 2017-12-28T10:02:43Z

@JaKXz @bcoe is there any known eta for this merge to make it into master?

damianobarbati · 2018-01-20T12:05:53Z

@bcoe 🙏🏻

SimenB · 2018-01-22T09:07:20Z

It would be great (for both this and Jest) to be able to have some way of adding all syntax plugins (not transforms), so that they are not a blocker when people try to use code supported by Node itself. Not sure if it's feasible (or a good idea), though... But having to explicitly add syntax plugins all the time when node add support for stage 2 stuff is somewhat painful.

/cc @loganfsmyth

damianobarbati · 2018-01-31T09:35:09Z

@SimenB no luck so far on this?

dcherman · 2018-03-09T18:16:22Z

Bump - it's been over a month since the last comment. Can we get some guidance from the maintainers here on expected ETA to merge/release?

bcoe · 2018-03-09T22:32:16Z

@SimenB @dcherman sorry for the slow turnaround my wife Cambi passed away recently and I've been putting less time into open-source than in the past.

I am starting to get back in the swing of things, and one thing I have planned is to start setting up a monthly in person/remote triage meeting for Istanbul, yargs, conventional-changelog, and the other libraries I maintain.

Please join the community here if you'd like to be involved.

@SimenB @loganfsmyth I like your idea of coming up with a more generic approach to adding additional syntax (in a generic way), prod me in chat, and I'll make an effort to be responsive to issues along this line.

bcoe · 2018-03-09T22:50:55Z

@SimenB please give babel-plugin-istanbul@4.1.6 a shot, if things are working smoothly we can promote on npm.

SimenB · 2018-03-18T22:27:09Z

@bcoe thanks for merging! The new version works perfectly 🙂

loganfsmyth · 2018-04-04T23:29:08Z

src/index.js

@@ -52,6 +53,7 @@ function makeShouldSkip () {
 function makeVisitor ({types: t}) {
  const shouldSkip = makeShouldSkip()
  return {
+    inherits: babelSyntaxObjectRestSpread,


This should really be reverted. If something needs to support object rest spread, the user should be the one opting into it, it shouldn't be a weird side-effect of loading istanbul. If Jest specifically wants to auto-enable it, this should be loaded by Jest.

@loganfsmyth @SimenB I'm keeping a close eye on babel/babel#7660 -- I agree it feels a little weird (and like a slippery slope) in hindsight to be enabling one specific language feature.

Getting state-4 language features by default sounds like the best possible scenario (is this already the case?).

Getting state-4 language features by default sounds like the best possible scenario

No disagreement there. We've mostly been focused on 7, but there's probably a good argument to do a last pass on Babel 6 to enable stabilized syntax by default.

Given that jestjs/jest#4519 landed in Jest, it seems like this doesn't have any reason to be in Istanbul though.

@SimenB can you verify this?

@loganfsmyth is there any reason we shouldn't be upgrading Istanbul to babel 7 as soon as possible? would this issue not arise in 7.

See this comment: jestjs/jest#5082 (comment)

Easy enough to do, but a new release of jest is also a bit out (we're gearing up for a new major), so it won't be fixed for user in a while

is there any reason we shouldn't be upgrading Istanbul to babel 7 as soon as possible?

It's probably good to update the plugin to support Babel 7 stuff, but we've done our best to make plugins work on both 6 and 7, so I think plugin should be careful not to drop support for 6 too fast.

would this issue not arise in 7.

The newer version of Babel is likely to support the newer standards certainly, so I'd expect it to have object spread on by default, but there will always be more stuff.

I'll also say that if babel/babel#7660 does become a thing, I'm not at all against Istanbul using it, but it seems like you'd want to be enabling it in instrumentSync rather than as part of the Babel plugin itself.

You've already got objectRestSpread hard-coded in https://github.com/istanbuljs/istanbuljs/blob/67918e2261a80acfafc0400b3980fafb6ae4aca5/packages/istanbul-lib-instrument/src/instrumenter.js#L89. If you changed that logic to use babel-core directly, it seems like things would be a lot simpler.

As a general overview of the current logic here and in instrument, I think there's a few things that seem surprising to me that complicate matters. Primarily, I think the current fact that the logic for the plugin doesn't actually live in the plugin leads to a lot of additional complexity. Mostly in the way that istanbul-lib-instrument directly depends on babel-generate and babylon, which are entirely unused via babel-plugin-istanbul. Similarly, babel-types is explicitly imported for Babel 6 here: https://github.com/istanbuljs/istanbuljs/blob/67918e2261a80acfafc0400b3980fafb6ae4aca5/packages/istanbul-lib-instrument/src/instrumenter.js#L6 but it is passed into the plugin here: https://github.com/istanbuljs/babel-plugin-istanbul/blob/master/src/index.js#L53 meaning that really the instrumentation logic should be using the version passed to the plugin, not importing it manually and locking yourselves to a specific version.

It seems like an ideal world we'd have

babel-plugin-istanbul as a standard Babel plugin that worked on Babel 6 and Babel 7 and contained all of the logic for injecting coverage tracking.

istanbul-lib-instrument which would wrap @babel/core@7.x and call it automatically with only babel-plugin-istanbul (potentially taking the user's local syntax plugins into account when parsing?)

SimenB mentioned this pull request Dec 17, 2017

Object spread error when collecting coverage with configured transform jestjs/jest#5082

Closed

fix: include object-spread-syntax plugin

f467aee

SimenB commented Dec 17, 2017

View reviewed changes

JaKXz requested a review from bcoe December 22, 2017 21:30

JaKXz approved these changes Dec 22, 2017

View reviewed changes

bcoe merged commit 428a952 into istanbuljs:master Mar 9, 2018

SimenB deleted the object-spread-syntax branch March 18, 2018 22:25

SimenB mentioned this pull request Apr 3, 2018

Create preset consisting solely of syntax plugins babel/babel#7660

Closed

loganfsmyth reviewed Apr 4, 2018

View reviewed changes

bcoe mentioned this pull request Jun 26, 2018

feat: Upgrade to babel 7, drop Node 4 support. #158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: include object-spread-syntax plugin #141

fix: include object-spread-syntax plugin #141

SimenB commented Dec 17, 2017 •

edited

SimenB Dec 17, 2017

coveralls commented Dec 17, 2017 •

edited

coveralls commented Dec 17, 2017

coveralls commented Dec 17, 2017 •

edited

JaKXz left a comment •

edited

damianobarbati commented Dec 28, 2017 •

edited

damianobarbati commented Jan 20, 2018

SimenB commented Jan 22, 2018

damianobarbati commented Jan 31, 2018

dcherman commented Mar 9, 2018

bcoe commented Mar 9, 2018 •

edited

bcoe commented Mar 9, 2018

SimenB commented Mar 18, 2018

loganfsmyth Apr 4, 2018

bcoe Apr 5, 2018

loganfsmyth Apr 5, 2018

bcoe Apr 5, 2018

SimenB Apr 5, 2018

loganfsmyth Apr 5, 2018

fix: include object-spread-syntax plugin #141

fix: include object-spread-syntax plugin #141

Conversation

SimenB commented Dec 17, 2017 • edited

SimenB Dec 17, 2017

Choose a reason for hiding this comment

coveralls commented Dec 17, 2017 • edited

coveralls commented Dec 17, 2017

coveralls commented Dec 17, 2017 • edited

JaKXz left a comment • edited

Choose a reason for hiding this comment

damianobarbati commented Dec 28, 2017 • edited

damianobarbati commented Jan 20, 2018

SimenB commented Jan 22, 2018

damianobarbati commented Jan 31, 2018

dcherman commented Mar 9, 2018

bcoe commented Mar 9, 2018 • edited

bcoe commented Mar 9, 2018

SimenB commented Mar 18, 2018

loganfsmyth Apr 4, 2018

Choose a reason for hiding this comment

bcoe Apr 5, 2018

Choose a reason for hiding this comment

loganfsmyth Apr 5, 2018

Choose a reason for hiding this comment

bcoe Apr 5, 2018

Choose a reason for hiding this comment

SimenB Apr 5, 2018

Choose a reason for hiding this comment

loganfsmyth Apr 5, 2018

Choose a reason for hiding this comment

SimenB commented Dec 17, 2017 •

edited

coveralls commented Dec 17, 2017 •

edited

coveralls commented Dec 17, 2017 •

edited

JaKXz left a comment •

edited

damianobarbati commented Dec 28, 2017 •

edited

bcoe commented Mar 9, 2018 •

edited