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

fix: Add flag to allow instrumentation of non-strict scripts. #863

Merged
merged 1 commit into from
Jul 5, 2018

Conversation

coreyfarrell
Copy link
Member

nyc was hard-coded to use esModules: true when running the
instrumenter. This could result in certain scripts not being
instrumented.

function package() {
  return 'package is a reserved keyword'
}

console.log(package())

Using --es-modules false will allow this script to be instrumented.

Issue #796

.option('es-modules', {
default: true,
type: 'boolean',
describe: 'Tell the instrumenter to treat files as ES Modules.',
Copy link
Member

Choose a reason for hiding this comment

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

no capital on Tell or . needed, to match style of other options.

@@ -115,6 +115,12 @@ Config.buildYargs = function (cwd) {
describe: 'cache babel transpilation results for improved performance',
global: false
})
.option('es-modules', {
default: true,
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to not default this to false, I guess I'm not fully understanding what this flag buys us -- since ES-modules aren't supported in vanilla JS yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

I set the default true to match the existing behavior. The biggest effect is that it puts the Babel parser into strict mode.

I can default to false if you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

If it makes us less sensitive, without sacrificing our ability to instrument anything, my temptation would be to default it to false ... what do you think?

perhaps someone from the babel project can chime in, CC: @loganfsmyth

Copy link
Member Author

Choose a reason for hiding this comment

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

A couple relevant details:

  1. The istanbul-lib-instrument default for esModules is false.
  2. This is translated into the babelrc option sourcetype 'module' or 'script'. The default for babel 6 and 7 is sourceType: 'module'.

So honestly I think istanbul-lib-instrument should also have esModules: true by default so that not specifying any option causes babel to behave as normal. I also understand the argument for being less restrictive, I don't really see this as a huge issue either way.

https://babeljs.io/docs/en/babel-core/#options
https://babeljs.io/docs/en/next/babel-core#options

Copy link

@loganfsmyth loganfsmyth Jun 27, 2018

Choose a reason for hiding this comment

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

Personally I think the JS ecosystem is in a bit of an unfortunate place here, since there's simply no way to 100% guarantee that you have the type of a file right without additional data. That said, in the context of nyc, and now that istanbuljs/babel-plugin-istanbul#158 has landed, it probably makes the most sense for nyc to default to unambiguous which you'll see in the next docs. It essentially means that the parser will guess the type.

Does Istanbul actually use the strictness of code the do anything during instrumentation? I'm assuming it doesn't. If that's the case, guessing wrong doesn't actually affect anything in Istanbul and just makes things more likely to parse.

Copy link
Member

Choose a reason for hiding this comment

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

@coreyfarrell let's default the setting to whatever setting is more permissive, it sounds like false? this seems more appropriate for Istanbul; we can always back-pedal on this decision if it results in any unexpected upstream errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bcoe Set to false and updated the tests.

nyc was hard-coded to use `esModules: true` when running the
instrumenter.  This could result in certain scripts not being
instrumented.

```js
function package() {
  return 'package is a reserved keyword'
}

console.log(package())
```

Add command-line option `--es-modules true` to force strict parsing.

Issue istanbuljs#796
@bcoe bcoe merged commit 6b6cd5e into istanbuljs:master Jul 5, 2018
@coreyfarrell coreyfarrell deleted the opt-es-modules branch July 6, 2018 00:14
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.

3 participants