fix: Add flag to allow instrumentation of non-strict scripts. #863
Conversation
coreyfarrell
force-pushed
the
opt-es-modules
branch
from
ec871b5 to
76d6e8e
Compare
lib/config-util.js
Outdated
| .option('es-modules', { | ||
| default: true, | ||
| type: 'boolean', | ||
| describe: 'Tell the instrumenter to treat files as ES Modules.', |
There was a problem hiding this comment.
no capital on Tell or . needed, to match style of other options.
lib/config-util.js
Outdated
| @@ -115,6 +115,12 @@ Config.buildYargs = function (cwd) { | |||
| describe: 'cache babel transpilation results for improved performance', | |||
| global: false | |||
| }) | |||
| .option('es-modules', { | |||
| default: true, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
A couple relevant details:
- The istanbul-lib-instrument default for esModules is false.
- This is translated into the babelrc option sourcetype
'module'or'script'. The default for babel 6 and 7 issourceType: '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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@bcoe Set to false and updated the tests.
coreyfarrell
force-pushed
the
opt-es-modules
branch
from
76d6e8e to
d41ce59
Compare
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
coreyfarrell
force-pushed
the
opt-es-modules
branch
from
d41ce59 to
3578a2b
Compare
nyc was hard-coded to use
esModules: truewhen running theinstrumenter. This could result in certain scripts not being
instrumented.
Using
--es-modules falsewill allow this script to be instrumented.Issue #796