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: adds support for source-map production #439

Merged
merged 7 commits into from
Nov 10, 2016
Merged

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Nov 7, 2016

@kpdecker I had some refactoring on my mind, that I opted to tack onto your work so that we can get this landed -- rather than overly marking up #393.

I made a few very minor changes:

  • I think that source-map production and the application of source maps, are different enough behavior that it's worth introducing the additional flag --produce-source-map; I also had some performance concerns, and think it's better that folks using source-map-support opt in.
  • I've been trying to gradually pull more and more logic out of index.js, so that it's ultimately just calling a bunch of pretty easy to follow libraries and helpers. tldr; I pulled the source-map logic into the instrumenter.
  • I added a couple TODOs around adding additional tests, and opened add more tests for produce-source-map logic #438.

CC: @JaKXz would love to have you take one final look at this.

kpdecker and others added 6 commits November 6, 2016 20:06
This allows the user to control the source map output post instrumentation.

Not fully straightforward right now, but when combined with a fork of source-map-support that has inline support (or placing the files in the proper location), end to end covered source maps can be achieved.

For runtime source-map-support, see evanw/node-source-map-support#118. Will be forking to @kpdecker/source-map-support soon, since the canonical project appears to be dead.
Simplify the API and add tests.
@bcoe bcoe mentioned this pull request Nov 7, 2016
@kpdecker
Copy link
Member

kpdecker commented Nov 7, 2016

Awesome. LGTM

@@ -128,7 +127,9 @@ NYC.prototype.instrumenter = function () {
}

NYC.prototype._createInstrumenter = function () {
return this._instrumenterLib(this.cwd)
return this._instrumenterLib(this.cwd, {
produceSourceMap: this.config.produceSourceMap
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

Fantastic. Thanks for calling out the branch on merging source maps.

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