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

Improve source map support #374

Merged
merged 1 commit into from
Feb 13, 2017
Merged

Improve source map support #374

merged 1 commit into from
Feb 13, 2017

Conversation

jhnns
Copy link
Member

@jhnns jhnns commented Feb 13, 2017

  • Make all source map paths relative to the entry file. This way we don't have to read the context option from the webpack config.

  • Fix source maps on Windows: node-sass returns POSIX paths, that's why we need to transform them back to native paths. This fixes an error on windows where the source-map module cannot resolve the source maps.

See #366 (comment)

- Make all source map paths relative to the entry file. This way we don't have to read the `context` option from the webpack config.

- Fix source maps on Windows: node-sass returns POSIX paths, that's why we need to transform them back to native paths. This fixes an error on windows where the source-map module cannot resolve the source maps.

See #366 (comment)
@jhnns
Copy link
Member Author

jhnns commented Feb 13, 2017

@bholloway this will again be breaking for the resolve-url-loader. I've created a PR for this: bholloway/resolve-url-loader#44

@jhnns jhnns merged commit e5c25af into master Feb 13, 2017
@jhnns jhnns deleted the refactor/source-map branch February 13, 2017 19:17
@bholloway
Copy link
Contributor

@jhnns Can you confirm that module imports (i.e. tilde ~) are relative to the entry file? I am seeing otherwise.

I will try to reproduce with your /test project.

@bholloway
Copy link
Contributor

@jhnns I have tweaked your bootstrapSass test to debug the source-maps.

The source code is in this gist. It requires you to install adjust-sourcemap-loader.

The overall output is of the form:

{
  "sources": [
    "webpack:///webpack:///test/scss/bootstrap-sass.scss",
    "webpack:///webpack:///test/scss/~/bootstrap-sass/assets/stylesheets/_bootstrap.scss"
    ...
  ]
  ...,
  "sourceRoot": ""
}

We can ignore the repeated webpack:/// protocol for now. It has been a while since I debugged source-maps but the tilde in the path certainly feels wrong.

I am less interested in the overall output and more interested in what goes into the resolve-url-loader.

The debug output from adjust-sourcemap-loader shows the source-map following sass-loader (i.e. at its input):

INPUT bootstrap-sass.scss
      node_modules/bootstrap-sass/assets/stylesheets/_bootstrap.scss
      node_modules/bootstrap-sass/assets/stylesheets/bootstrap/_variables.scss
      ...

While this debug info does not include the sourceRoot I believe a "sourceRoot": "test/scss" would produce the overall output we see above. However you can see there is no common source root possible for these paths.

(Please Note the adjust-sourcemap-loader will try to locate files using all the CODECs at its disposal. That is the ABSOLUTE path in the debug output. However that does not help us here.)

jhnns added a commit that referenced this pull request 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)
@jhnns
Copy link
Member Author

jhnns commented Feb 14, 2017

These source maps are truly driving me crazy ^^

Could you review #377 and try it with your test setup? I think, now all paths should be correct.

@bholloway
Copy link
Contributor

@jhnns There is good news and bad news. I will comment on #377 because it is still open.

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