joliss / broccoli-sass

Sass compiler for Broccoli, using libsass
MIT License
44 stars 97 forks source link

Added info about source maps to README.md #19

Closed simonexmachina closed 10 years ago

simonexmachina commented 10 years ago

@joliss, just wanted to run this by you before adding this to upstream. You okay for me to merge this in?

joliss commented 10 years ago

I worry that this is too much of a hack, to be honest. If we document it, then we're basically blessing it as an official workflow and have to support it.

It would seem that the "proper" solution is to either pass the source files along, making sure that references to the file path stay intact, or inline them with a large data URL. I haven't researched how this would work in practice, though.

What do you think?

simonexmachina commented 10 years ago

No, I think this is correct. If you want to include the source files in the output then it will work as expected, it's just that you generally don't want to do that - and this is the way to make it work. Also it just feels hacky because it's GUI :) But that interaction is actually the correct and proper way to tell the browser where to find the source files, and for people who use this feature already they'd instantly recognise it. It's really to help out newcomers and save them creating an issue.​

joliss commented 10 years ago

The more I think (and tweet) about this, the more I believe we really need to serve out the original source file in some form.

Because Broccoli allows you to preprocess files and move them around, there's in general no reason to assume that the input files to Sass will exist in the project directory - they might be hidden in some intermediate tmp dir. There's also the problem that as you add vendor directories for your include paths (inputTrees), there'll be more and more directories to link up in Chrome. So I think the "map to file system" thing is really a workaround.

joliss commented 10 years ago

I'd really like source maps to "just work" at some point, but it seems there's a bit more work to be done before this will happen.

simonexmachina commented 10 years ago

Okay, I've been thinking about this, and I see where you're coming from. I'm just not sure how that'd work: are we saying that developers need to ensure that they create trees to copy all of the input trees into their output tree? This could be complicated, and it may be better for us to provide an option in broccoli-sass to do this for them automatically? For example, in ember-cli the developer doesn't really have access to the final input trees in order to put these into the output tree.

I think source maps do "just work" in this module, but source maps in general depend on the original source files being available to the user agent (am I wrong about this?). Whether they're available over HTTP (ie. included in the output tree) or via a filesystem mapping is an implementation detail to me.

My preference is to include this info, but I'm happy to leave it out and let people work it out for themselves. I respect your decision not to include this because it's "a workaround", but my preference would be to provide the info.

Re. "problem that as you add vendor directories for your include paths (inputTrees), there'll be more and more directories to link up in Chrome" - I'm pretty sure it supports multiple mappings for a given workspace, so you can just add the whole project and then set up the mappings for different folders as required.

joliss commented 10 years ago

I'm unsure how it would work. Clearly you'd need some way or convention to ensure that references and files don't get lost on the way.

Or perhaps inline the source files into data URIs. Not sure what kind of performance implications that has, and what the state of the tooling is, but it doesn't seem fundamentally unreasonable.

If I wanted to move the source map thing forward, I'd probably look around (on Google + Twitter) for people actively working on the the fundamentals, like the spec and the source map libraries. It seems to me there are some architectural things to be solved, especially when we want to support not just generating but also transforming source maps, i.e. for chained compilers. It's clearly a worthwhile thing to solve.

simonexmachina commented 10 years ago

Okay, so one way it could work would be to add an option .includeSource which merges the input trees with the output tree that contains the outputFile.

Inlining the source is horrible :) I've used it before and from memory is was slow and horrible. That was a while ago, happy to pursue this if someone thinks it's the way to go.

Do you know anyone who's an expert on source maps? Maybe you could ask on Twitter? (I don't have many followers) AFAIK they rely on the source files being available (over HTTP or the via the user agent from the file system).

joliss commented 10 years ago

Okay, so one way it could work would be to add an option .includeSource which merges the input trees with the output tree that contains the outputFile.

I'm a bit unsure how we're going to serve that out in the end - we probably don't want random JS files floating around in the main output dirs - and how to keep the references intact.

Inlining the source is horrible :) I've used it before and from memory is was slow and horrible.

Might be worth figuring out if this is a fundamental thing, or if the bottleneck can be optimized there.

Do you know anyone who's an expert on source maps?

I'd probably just go for the authors of the source map spec, https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit. Nick also wrote most of the source-map code.

Given that there's still a lot of things to be figured out, I don't really want to attempt merging any kind of "blessed" source map support into broccoli-sass yet, but perhaps we can put it on a branch or so, and then iterate from there.

simonexmachina commented 10 years ago

Okay, so one way it could work would be to add an option .includeSource which merges the input trees with the output tree that contains the outputFile. I'm a bit unsure how we're going to serve that out in the end - we probably don't want random JS files floating around in the main output dirs - and how to keep the references intact.

Yes, I was meaning mention that we'd probably need to put these into a specific directory. Here's my thinking:

Then consumers can set .includeSource to have the source files included in their output.

What do you think?

Given that there's still a lot of things to be figured out, I don't really want to attempt merging any kind of "blessed" source map support into broccoli-sass yet, but perhaps we can put it on a branch or so, and then iterate from there.

Okay, I'll speak to Nick to a) to confirm that source maps need to have the source files included in the build output, and b) ask whether it's a good solution to inline the source into the output file.

joliss commented 10 years ago

Maybe. I'd really like to get things working without configuration though. Any time a user has to manually wire up source maps, it seems like we're doing something wrong.

Re the directory, how about defaulting to

outFile.css
outFile.css.map
outFile.css.sources/    (or even outFile.css.map.sources/)
  sourceFile.scss
  ...

Is this strategy generally good enough to keep things "just working" across chained transformations? The chaining use case is as of yet rare in CSS land, but very common in JavaScript land. Like when you run multiple preprocessors in a row, the second one will actually consume the source map(s) emitted by the first one and transform it into a new source map.

simonexmachina commented 10 years ago

Maybe. I'd really like to get things working without configuration though. Any time a user has to manually wire up source maps, it seems like we're doing something wrong.

Yeah I get that, but for me it's not wiring them up - just opting-in for them to be generated.

outFile.css.sources/ is a good default - we could use this when .includeSources === true. So, would we also provide the ability to specify the directory, maybe using .includeSources as a string?

Is this strategy generally good enough to keep things "just working" across chained transformations?

I would think so, but I just don't know. Subsequent consumers would need to know where to get the source map, and then add to that if it exists. Likewise with the sources, they could add to the .sources/ directory. Overwriting source files from previous steps is a problem but I can't see a case where this would happen.

I think a good naming convention is sufficient, and the .map and .sources/ suffixes provide a reasonable convention for this.

simonexmachina commented 10 years ago

I started a spike to see if we could add the sourcesContent to the source map ourselves, but the problem is that node-sass includes the file paths in the sources array (eg. node_modules/node-neat/node_modules/node-bourbon/assets/stylesheets/_bourbon.scss) rather than the relative path that's specified in the @import statement (eg. _bourbon.scss for @import "bourbon").

So when I search the includePaths for the source file it's not found. For example, when sources includes node-neat/node_modules/node-bourbon/assets/stylesheets/_bourbon.scss this is the error:

Error: File not found: node-neat/node_modules/node-bourbon/assets/stylesheets/_bourbon.scss
in any of the following include paths:
  assets
  node_modules/node-neat/node_modules/node-bourbon/assets/stylesheets
  node_modules/node-neat/assets/stylesheets
joliss commented 10 years ago

I honestly think the best strategy is to figure out the C code and send a patch to libsass. Bolting the sourcesContent thing on in downstream plugins (like node-sass or broccoli-sass) seems much more error-prone to me.

the problem is that node-sass includes the file paths in the sources array

I agree that this is rather suboptimal behavior. It seems like it would make sense to fix libsass to put the relative path here.

simonexmachina commented 10 years ago

Yeah, I totally agree. But unfortunately I have 0 C experience, so I don't think I'm the man for the job. That said, I doubt it's a difficult change... I'll have a look.

simonexmachina commented 10 years ago

Okay, I've had a look. It looks pretty easy, just need to handle the JSON encoding for the sourcesContent and I have no idea how to import a lib to handle that. Looks like this lib will do what we need. I'll ask a friend of mine who knows stuff about C.

simonexmachina commented 10 years ago

Okay, I had a go at adding support to node-sass. Pretty sure it's (mostly) correct, but it segfaults when I run it from node-sass :(

simonexmachina commented 10 years ago

We're currently working on making this just work, but people have been asking how to do this. In the interim you can use these instructions.