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

Fix incorrect source maps in certain cwds #377

Merged
merged 1 commit into from
Feb 15, 2017
Merged

Fix incorrect source maps in certain cwds #377

merged 1 commit into from
Feb 15, 2017

Conversation

jhnns
Copy link
Member

@jhnns jhnns commented Feb 14, 2017

All source map paths will be relative to process.cwd() from now on.
This removes also the last dependency on this.options.context.
node-sass source map options like sourceMapRoot, omitSourceMapUrl, sourceMapContents are now overridable.

#374 (comment)

All source map paths will be relative to process.cwd() from now on.
This removes also the last dependency on this.options.context.
node-sass source map options like sourceMapRoot, omitSourceMapUrl, sourceMapContents are now overridable.

#374 (comment)
@jhnns jhnns mentioned this pull request Feb 14, 2017
@jhnns
Copy link
Member Author

jhnns commented Feb 14, 2017

@bholloway I've written a test that checks if all files in the source map can be resolved.

@bholloway
Copy link
Contributor

@jhnns I wish there was some standard for this because every loader does it differently. Some are really broken on windows too.

I think this will work. When content is not embedded then it will need to be served to the browser. So we should avoid ../ paths in the source-map. The shallowest common directory is usually process.cwd() (or close to it) so I think you are safe there.

I learned of some users that have sources outside process.cwd(). And with npm link this can be easy to do. But I think these are the edge cases that maybe cannot (easily) be served anyway.

Interestingly (in Webpack 1 at least) all of this is unlikely to influence the final source-map. That is done by devtoolModuleFilenameTemplate.

@bholloway
Copy link
Contributor

@jhnns
The good news:

I believe this fixes the source-map sources inconsistency from the original comment.

I have tested the hacked version of your Webpack 2 project that includes some resolve-url-loader processing.

I say MERGE!

The bad news (not your problem):

I believe that node-sass@>=4 has a problem with the source map. There are negative original mappings. For example:

"original":{"line":15,"column":-12}

When I made the #374 comment I believed that it did not effect resolve-url-loader.

However it seems like it is a problem for resolve-url-loaderin in my Webpack 1 project. I have inserted some resolve-url-loader processing in your Webpack 2 tests and that is not a problem.

Hopefully there is some error in my Webpack 1 test methodology and resolve-url-loader is immune to this as I originally had hoped.

Have you encountered this? I think this is not your problem, or at least, a different issue.

@jhnns jhnns merged commit ad816d1 into master Feb 15, 2017
@jhnns jhnns deleted the fix/source-map branch February 15, 2017 13:12
@jhnns
Copy link
Member Author

jhnns commented Feb 15, 2017

Thx for reviewing it.

I believe that node-sass@>=4 has a problem with the source map. There are negative original mappings. For example:

That is a known problem. I don't know enough about source maps, whether that is valid. But it seems like the source maps from node-sass itself are interpreted correctly (tested on chrome). I assume that webpack's source-map module has problems with this, but I'm not sure.

When content is not embedded then it will need to be served to the browser. So we should avoid ../ paths in the source-map. The shallowest common directory is usually process.cwd() (or close to it) so I think you are safe there.

Chrome allows to configure a directory mapping for source maps. Thus, source maps don't need to be served.

The problem here is that we don't know anything about the final source map destination inside the loader. That's why I think it's better to use relative paths with a sourceRoot. This way, the browser should be able to locate the actual source file on disk (only exception: The file is on a different hard drive).

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.

2 participants