Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Add force-sourcemap to embed inline sourcemaps #25

Closed

Conversation

zakhenry
Copy link

@zakhenry zakhenry commented May 20, 2016

This allows a flag to be added to force the embedding of an inline sourcemap to the file. This resolves issues with webpack not outputting sourcemaps for tools like remap-istanbul to function.

I have published this separately at https://www.npmjs.com/package/sourcemap-istanbul-instrumenter-loader if you want to test

@TheLarkInn
Copy link

TheLarkInn commented Jul 15, 2016

@zakhenry do you perhaps have an example repo that contains typescript with webpack and coverage like this? Is remap-instanbul also required?

@zakhenry
Copy link
Author

@TheLarkInn yea this is used in https://github.com/ubiquits/toolchain/blob/master/cli/tasks/test.js and yes remap istanbul is required. Note that the setup isn't trivial though as I merge two coverage reports from two sources (frontend and backend unit tests)

@@ -19,5 +19,24 @@ module.exports = function(source) {
this.cacheable();
}

// if force sourcemap requested embed inline sources (helps resolve issues with
// transpiled languages like typescript
if (userOptions['force-sourcemap']) {

Choose a reason for hiding this comment

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

Is there a reason this needs to be hidden behind a flag? Can't this simply be:

if (sourceMap) {
    var encoded = new Buffer(JSON.stringify(sourceMap)).toString('base64');
    source += '\n\n//# sourceMappingURL=data:application/json;base64,' + encoded;
}

If someone wants to ignore source maps, they can use exclude.

Copy link
Author

Choose a reason for hiding this comment

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

This is more of a hack as there may actually be standalone source maps alongside, but they don't seem to work for reasons I forget. This pr allows forcing an inline sourcemap to be appended, I don't think its actually a good long-term solution

Choose a reason for hiding this comment

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

When you say, "standalone source maps alongside," do you mean source maps that aren't inlined but are in, for instance, a .js.map file?

Copy link
Author

Choose a reason for hiding this comment

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

Yea that's the one, though with webpack I'm a bit fuzzy with what point the source maps are output

Copy link
Author

Choose a reason for hiding this comment

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

It's been a while since I did this PR so the details have escaped me

Choose a reason for hiding this comment

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

From my understanding, the second parameter (sourceMap) will be null or undefined unless a previous loader in the chain has provided one. Most transpiling loaders provide the sourceMap parameter, but for third-party code (ex. Angular 2, bootstrap, etc.) you have to use source-map-loader. source-map-loader will load both inline and external source maps, which should solve the problem you were seeing. Even so, if the second parameter of the loader isn't provided, a sourceMappingURL shouldn't be generated for the code since it will only cause webpack to process an object that it shouldn't even have to look at.

@bryanforbes
Copy link

@deepsweet Is there any way we can get you to take a look at this pull request? I don't think the logic needs to be behind a flag/property, but this issue is preventing instrumented transpiled code from being able to be remapped back to the original code via remap-istanbul.

@samvloeberghs
Copy link

@bryanforbes, @deepsweet seems to be active on github as he has activity, probably notifications turned off or something. Anyway I tweeted him as I need this as well! https://twitter.com/samvloeberghs/status/773126385074311168

@deepsweet
Copy link
Collaborator

hey guys. I'll merge #28 later today.

@MichaReiser MichaReiser mentioned this pull request Sep 14, 2016
@deepsweet deepsweet closed this in d49b054 Nov 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants