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

Source map paths (and sourceRoot support) #908

Closed
joliss opened this issue Feb 27, 2015 · 9 comments
Closed

Source map paths (and sourceRoot support) #908

joliss opened this issue Feb 27, 2015 · 9 comments

Comments

@joliss
Copy link

joliss commented Feb 27, 2015

Edit: Scenario in a nutshell is: "We're generating output into a dist directory to be deployed on a web server (for development or production). We want source maps to just work."

Hey all,

Thanks for your awesome work on libsass. I'm working on broccoli-sass (which in turn is used by ember-cli), and I'm trying to get the source map support right.

Unfortunately, the paths that are emitted in source maps are somewhat suboptimal for us. I made a minimal example to illustrate this at https://github.com/joliss/node-sass-source-map-example, which I'll walk you through here:

Running standard-output.js to compile styles/app.scss into dist/assets/app.css, we get the following source map:

$ node standard-output.js
{
    "version": 3,
    "file": "app.css",
    "sources": [
        "../../app.scss",        <---------------------- these paths are the problem
        "../../../vendor/bootstrap.scss"
    ],
    "sourcesContent": [
        "@import \"bootstrap\";\n\nbody {\n  background-color: green;\n}\n",
        "body::before {\n  content: \"Hello world\";\n}\n"
    ],
    "mappings": "ACAA,AAAI;EACF,AAAS;;ADCX;EACE,AAAkB",
    "names": []
}
Serving ./dist on http://0.0.0.0:3000/

When we serve dist using a web server, rather than using file:/// URLs in the browser, paths like "../../app.scss" that point outside the dist directory obviously cannot resolve.

As a result, the folder structure in Chrome devtools ends up being all messed up - we expect styles/app.scss and vendor/bootstrap.scss, but get the following:

devtools screenshot 1

(If you want to try this yourself, note that to you need to enable CSS source maps in the devtools settings.)


As to what we should be emitting instead, I'm not entirely clear, but how about the following source map output (implemented in better-output.js):

$ node better-output.js
{
    "version": 3,
    "file": "app.css",
    "sources": [
        "styles/app.scss",   <----------------------------- better
        "vendor/bootstrap.scss"
    ],
    "sourcesContent": [
        "@import \"bootstrap\";\n\nbody {\n  background-color: green;\n}\n",
        "body::before {\n  content: \"Hello world\";\n}\n"
    ],
    "mappings": "ACAA,AAAI;EACF,AAAS;;ADCX;EACE,AAAkB",
    "names": [],
    "sourceRoot": "file:///home/ubuntu/src/node-sass-source-map-example"  <------- added
}
Serving ./dist on http://0.0.0.0:3000/

We're also adding a "sourceRoot" key pointing at our repo root, so that Chrome devtools will sort the .scss files separately from the .css files. This is what it looks like now:

devtools screenshot 2

Chrome doesn't seem to be smart enough quite yet to pick up on the file:/// sourceRoot URL and automatically suggest it as a workspace, so that we can edit and save the .scss source files right there in devtools, but I don't see a reason why this couldn't be added in the future.

At the moment, libsass makes the assumption that "sources" paths should be relative to the source map file. The example output I'm generating here with better-output.js rather makes the paths relative to the project root ("sourceRoot"), and tries to display them in a separate file hierarchy in devtools. I had a hangout with @stefanpenner (from ember-cli) today, and we had some agreement that this seems like a generally reasonable way to do it.

The better-output.js code uses some hacks to make this happen. Obviously if we want to support this kind of source map output, libsass would need to add proper support for it.

What are your thoughts?

@joliss joliss changed the title Source map paths and sourceRoot support Source map paths (and sourceRoot support) Feb 27, 2015
@mgreter
Copy link
Contributor

mgreter commented Feb 27, 2015

IMO this is clearly a job for postprocessors.

Running standard-output.js to compile styles/app.scss into dist/assets/app.css ...
When we serve dist using a web server ...

This gives it away that the source file "app.scss" is indeed not accessible if you only serve dist. This is not really something libsass can solve. From your example I don't see how either Asset can actually be accessed, if you only serve "dist"!? Also, If we would support this, it would break the paths for other postprocessors! AFAIK the whole source-path stuff has also been verified by @am11 to work correctly the way it is now.

@am11
Copy link
Contributor

am11 commented Feb 27, 2015

@joliss, it seems like the issue is due to the reason node-sass require user to provide abs. path for input file, while we should resolve it for the user (in case of relative path); the same way we resolve outFile.

The outFile is then resolved with respect to (input) file and source-map path is resolved with respect to outFile.

Can you please open this issue in node-sass instead?

Thank you.

@am11
Copy link
Contributor

am11 commented Feb 27, 2015

I just realized that libsass is resolving the input path with respect to cwd, so we don't have to do that. Please ignore my previous comment. :)

@joliss
Copy link
Author

joliss commented Feb 27, 2015

This gives it away that the source file "app.scss" is indeed not accessible if you only serve dist

Exactly. This appears to be a relatively common scenario; let me state it explicitly: "We're generating a dist directory to be deployed on a web server (for development or production). We want source maps to work."

It seems to me that the "sourcesContent" key was added to the source map spec to address the issue of sources (like app.scss) not being available, by embedding everything in the .map file. (I also have "sourcesContent" enabled in my examples above.) The path names listed in "sources" are therefore mostly "decorative" in that they affect how paths are displayed in devtools; we still need to get them right though.

It's not clear to me at this point what the expected correct behavior even is, and that's the question I'm raising with this issue. Like, what do we want the source map to look like in a scenario like this? Is the source map generated by "better-output.js" in my example really the right way, with a file:/// URL for sourceRoot and all? I'm a bit dubious.

We would like to be able to have good source map support in downstream packages like broccoli-sass to make it "just work", but at the moment we don't really know what the correct output for this scenario would even look like. Perhaps it might make sense to link up with people working on the Firefox or Chrome devtools as well (does Paul Irish still work on this stuff?)... I'm not volunteering unfortunately, but I just wanted to bring up this problem.

@am11
Copy link
Contributor

am11 commented Feb 27, 2015

@joliss,

The path related issues usually belong to node-sass, since LibSass (for most part) has nailed the path related aspect of source-map.

The node-sass API's behavior is:

  • Both .map and .css paths are resolved with respect to input path.
  • Both .map and .css are placed in the same directory, when sourceMap: true. If you want to place it somewhere else, just set sourceMap: 'your/preferred/path'.

So node-sass provides following options for your scenario:

1 - Provide correct relative paths:

var result = sass.renderSync({ 
  file: 'styles/app.scss',   // <-- cwd + style/app.scss
   includePaths: ['styles', 'vendor'], 
   sourceMap: '../anyName.css.map',   // <--- relative to input path, could be any location.
   outFile: '../dist/assets/app.css',  // <-- also relative to input path
   sourceMapContents: true
}) 

2 - Provide absolute paths:

var result = sass.renderSync({ 
  file: '/path/to/my/project/app.scss', 
   includePaths: ['styles', 'vendor'], 
   sourceMap: '/path/to/my/project/app.css.map',
   outFile: '/path/to/my/project//dist/assets/app.css', 
   sourceMapContents: true
}) 

3 - Use sourceMap: true, but save it to the correct path (which appears in sourceMappingURL in generated CSS, which is how browser finds the map file):

var result = sass.renderSync({ 
  file: 'styles/app.scss', 
   includePaths: ['styles', 'vendor'], 
   sourceMap: true,
   outFile: '../dist/assets/app.css',
   sourceMapContents: true
}) 

writeFile(result.map, '../dist/assets/app.css.map') // but understandably, 
                                                    // this path hard to calculate,
                                                    // or worse solution could be to parse CSS
                                                    // for sourceMappingURL using regex
                                                    // which won't work, if you set
                                                    // omitSourceMapUrl to false in 
                                                    // node-sass options
                                                    // hence option 4 below

4 - Wait for node-sass vNext where you will be able to get back the resolved sourceMap path in callback, when using render (async) method: sass/node-sass#686

sass.render({ 
  file: 'styles/app.scss', 
   includePaths: ['styles', 'vendor'], 
   sourceMap: true,
   outFile: '../dist/assets/app.css',
   sourceMapContents: true
}, function(err, result) { // node-style cb, new in vNext
  writeFile(result.map, this.options.sourceMap)  // this.options is provided with resolved paths
}) 

Hope this helps.

@mgreter
Copy link
Contributor

mgreter commented Mar 2, 2015

IMHO OP is "moving" files after the compilation (even if virtually via httpd configs). That's why I suggested that this is a job for a post-processor. I'm sorry but I probably going to close this as "wontfix".

You have this file structure on compile:

- dist/ (empty)
- styles/app.scss
- vendor/bootstrap.scss

you compile this to

- dist/assets/app.css

Now you serve dist and of course the links to the other assets don't work, because they are not accessible if you only serve dist. You do not even give away under which url you expect i.e. styles/app.scss to be accessible (is it even somehow accessible)! If your assets would be under dist when you compile everything, it should actually work!

I'm going go close this until we get further information!

@mgreter mgreter closed this as completed Mar 2, 2015
@simonexmachina
Copy link
Contributor

IMHO the correct solution to this problem is #668, in which libsass appends to a provided source map. This allows broccoli-sass to provide a source map that maps back to the source files, and libsass adds the mappings for the SASS to CSS transformation. This also has the added benefit of solving the write problem as well.

@xzyfer
Copy link
Contributor

xzyfer commented Mar 3, 2015

IMHO for us it comes down to "what does Ruby Sass do" our aim is to be a drop in replacement for Ruby Sass. If this behaviour differs it should be addressed.

@chriseppstein
Copy link
Contributor

Sourcemap links outside the "served" directory are not a problem when using chrome (not sure about firefox). When you set up a "development workspace" it lets you map a resource that is served over HTTP to a resource on disk, then relative paths in sourcemaps are resolved on the filesystem relative to the filesystem resource instead of the http resource.

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

No branches or pull requests

6 participants