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

feat: add es-modules option to instrument command #1006

Merged

Conversation

AndrewFinlay
Copy link
Contributor

This is pretty straightforward, allow the es-modules option to flow through to the instrumenter when running the standalone instrument command.

Tests adapted from the existing tests for the regular es-modules command.

This is pretty straightforward, allow the es-modules option to flow through when running the standalone instrument command.
Tests adapted from the existing tests for es-modules.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.639% when pulling aa36ab2 on AndrewFinlay:instrument-es-modules-option into b64d921 on istanbuljs:master.

@coreyfarrell
Copy link
Member

This might be a breaking change as currently nyc instrument file.js effectively has --es-modules=false. Not sure this is an issue, we might already be looking at pushing forward with NYC@14 anyways due to some of the include/exclude processing changes.

@coreyfarrell coreyfarrell requested review from bcoe and JaKXz March 7, 2019 12:12
@coreyfarrell
Copy link
Member

@bcoe @JaKXz I think setting the default true is correct as it matchs other parts of NYC, should this be flagged as:

BREAKING CHANGE: `nyc instrument` now enables the `--es-module` option by default.

BTW one of the projects I work with uses gerrit for code review, one of the really nice things is that the commit message is part of the review (not suggesting gerrit is viable, just wish github had a similar process as it would work really well for conventional-commits). So anyways please let me know what you think of the breaking change text I stated above (or if you think this should not be marked breaking). This can matter for sources such containing function package() {} or other keywords forbidden by 'use strict', the new default will be to fail instrumentation on sources that violate certain 'use strict' rules.

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

looks good to me 👍 I'm excited that the instrument command is getting some attention.

@coreyfarrell coreyfarrell merged commit 596b120 into istanbuljs:master Mar 7, 2019
@AndrewFinlay AndrewFinlay deleted the instrument-es-modules-option branch March 11, 2019 22:51
@coreyfarrell coreyfarrell mentioned this pull request Apr 3, 2019
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.

None yet

4 participants