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.

Sign up for GitHub

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

Jump to bottom

fix: include object-spread-syntax plugin #141

Merged
merged 1 commit into from
Mar 9, 2018
Merged

fix: include object-spread-syntax plugin #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@
"lib"
],
"dependencies": {
"babel-plugin-syntax-object-rest-spread": "^6.13.0",
"find-up": "^2.1.0",
"istanbul-lib-instrument": "^1.7.5",
"istanbul-lib-instrument": "^1.8.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

Explicitly bumping to ensure istanbuljs/istanbuljs#82 is included

"test-exclude": "^4.1.1"
},
"devDependencies": {
Expand Down
4 changes: 3 additions & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {realpathSync} from 'fs'
import {dirname} from 'path'
import {programVisitor} from 'istanbul-lib-instrument'
import babelSyntaxObjectRestSpread from 'babel-plugin-syntax-object-rest-spread'

const testExclude = require('test-exclude')
const findUp = require('find-up')
Expand All @@ -23,7 +24,7 @@ function makeShouldSkip () {
let config = {}
if (Object.keys(opts).length > 0) {
// explicitly configuring options in babel
// takes precendence.
// takes precedence.
config = opts
} else if (nycConfig.include || nycConfig.exclude) {
// nyc was configured in a parent process (keep these settings).
Expand Down Expand Up @@ -52,6 +53,7 @@ function makeShouldSkip () {
function makeVisitor ({types: t}) {
const shouldSkip = makeShouldSkip()
return {
inherits: babelSyntaxObjectRestSpread,
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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?).

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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?)

visitor: {
Program: {
enter (path) {
Expand Down