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

Create sourcemap in instrumented files #674

Merged
merged 1 commit into from
Oct 15, 2017
Merged

Conversation

schutm
Copy link
Contributor

@schutm schutm commented Sep 12, 2017

If invoked with nyc instrument it wasn's possible to attach the sourcemap. This created problems for me in the coverage reports created.

@bcoe
Copy link
Member

bcoe commented Sep 16, 2017

@schutm I'm not sure what the best solution is here such that you and @vanduynslagerp don't consistently break each-other's use-cases.

Is there a heuristic approach we could use so that both your applications of shouldInstrument work? I think we should make an effort to add a test for both so that we don't keep flip-flopping on this feature.

@schutm
Copy link
Contributor Author

schutm commented Sep 16, 2017

Agreed we shouldn't break each other changes, and test-cases should be there. However since this is in the CLI I didn't think it would break the previous changes.

For now seems I had a dangling comma; which I'll fix

@coveralls
Copy link

coveralls commented Sep 17, 2017

Coverage Status

Changes Unknown when pulling be893c5 on schutm:master into ** on istanbuljs:master**.

@bcoe
Copy link
Member

bcoe commented Sep 17, 2017

@schutm I think this accidentally reverts https://github.com/istanbuljs/nyc/pull/667/files which would break @vanduynslagerp's use-case.

The instrument command is now able to create an instrumented output, with a sourcemap attached.
@coveralls
Copy link

coveralls commented Sep 19, 2017

Coverage Status

Changes Unknown when pulling 6f83f56 on schutm:master into ** on istanbuljs:master**.

@schutm
Copy link
Contributor Author

schutm commented Sep 19, 2017

@bcoe I removed the accidentally included index.js, which reverted #667. In addition I tidied the commits up to a single one.

@bcoe
Copy link
Member

bcoe commented Oct 15, 2017

@schutm this looks great 👍 sorry that this took so long to land.

@bcoe bcoe merged commit f31d7a6 into istanbuljs:master Oct 15, 2017
@bcoe
Copy link
Member

bcoe commented Oct 23, 2017

@schutm try this out, and let me know if it does the trick:

npm i nyc@next --save-dev

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