search

joliss / broccoli-sass

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

Added support for sourcesContent and update to node-sass v2.0.1 #43

Closed simonexmachina closed 9 years ago

ryanto commented 9 years ago

Nice work.

Is there anything blocking this from merging? node-sass@2.0.0-beta is needed to for CentOS6.4 / gcc 4.4.7. See: https://github.com/sass/node-sass/issues/517

simonexmachina commented 9 years ago

I believe there's a beta version of broccoli-sass for this already, but if not @joliss should be able to release one

Globegitter commented 9 years ago

Would love to see this getting merged. Otherwise broccoli-sass is not working with iojs.

simonexmachina commented 9 years ago

@joliss is looking into this now, hopefully should have a release soon.

joliss commented 9 years ago

If I try to use { sourceMap: true, sourceMapEmbed: true } to generate an embedded source map, it still emits a .map file as well, so the source map duplicated (both inline and .map). Am I using it wrong?

simonexmachina commented 9 years ago

I'd guess that you would just provide sourceMapEmbed: true if you don't want the .map file. Do you want me to confirm that?

simonexmachina commented 9 years ago

Okay that's fixed now @joliss.

fivetanley commented 9 years ago

@aexmachina 2.0.0 final is out too now! https://github.com/sass/node-sass/releases/tag/v2.0.0

simonexmachina commented 9 years ago

Awesome, great work. I'd push an update, but the install fails:

> node-sass@2.0.0 install /Users/simonwade/dev/Workspace/broccoli-sass/node_modules/node-sass
> node scripts/install.js

Can not download file from https://raw.githubusercontent.com/sass/node-sass-binaries/v2.0.0/darwin-x64-iojs-0.10/binding.node

> node-sass@2.0.0 postinstall /Users/simonwade/dev/Workspace/broccoli-sass/node_modules/node-sass
> node scripts/build.js

module.js:340
    throw err;
          ^
Error: Cannot find module '/Users/simonwade/dev/Workspace/broccoli-sass/node_modules/node-sass/node_modules/pangyp/bin/node-gyp'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:929:3
Build failed
simonexmachina commented 9 years ago

@joliss node-sass 2.0.0 is released now. I've updated the PR and this is working well for us now. Would you please merge this and publish a new version of broccoli-sass. I know there's a few people who would like to use it.

Ghostavio commented 9 years ago

Guys right now it's impossible to run npm install on existing ember projects since it fails when trying to install node-sass. It would be very good to have this out or some other hotfix as soon as possible as ember is completely unusable right now :/

Globegitter commented 9 years ago

@Ghostavio you can just install broccoli-sass from this PR via npm i git+https://github.com/aexmachina/broccoli-sass#sources-content --save

simonexmachina commented 9 years ago

Yes unfortunately I don't have permission to publish the npm module. @joliss lots of people are waiting for this to be published, can you pretty please push a new version?

On Sat, 14 Feb 2015 08:12 Markus Padourek notifications@github.com wrote:

@Ghostavio https://github.com/Ghostavio you can just install broccoli-sass from this PR via npm i git+ https://github.com/aexmachina/broccoli-sass#sources-content --save

— Reply to this email directly or view it on GitHub https://github.com/joliss/broccoli-sass/pull/43#issuecomment-74327685.

am11 commented 9 years ago

@joliss,

If I try to use { sourceMap: true, sourceMapEmbed: true } to generate an embedded source map, it still emits a .map file as well, so the source map duplicated (both inline and .map). Am I using it wrong?

You are right it should produce map-file-content xor embedded-map-content. I have added that to https://github.com/sass/libsass/issues/885 with NEW tag (//cc @xzyfer). Unfortunately, @aexmachina's suggestion does not work because (of that flakiness described in that LibSass issue and) without setting sourceMap, sourceMapEmbed does not produce even source-maps (see other proposed specs in aforementioned issue).

Having said that, node-sass does not write to anything to file system (except for the CLI usage). Consider this implementation of node-sass:

// this is node interactive console
require('node-sass').info(); // 'node-sass version: 2.0.1\nlibsass version: 3.1.0'
require('node-sass').render({
  data: 'a{b:c}', // OR you can use file: /path/to/file.scss,
                  // but in case of `data` AND `file`, `data` will take precedence.
                  // more on this is described here: 
                  // https://github.com/sass/node-sass-middleware/issues/23#issuecomment-74374504

  outFile: 'my-prospective-output.css',  // again see sass/libsass#885 
                                         // it should 'only' be necessary when
                                         // separate source-maps (.map) is required.
                                         // if you are curious why it is necessary at all
                                         // (since node-sass is not dealing with IO)
                                         // see https://github.com/sass/node-sass/issues/591

  sourceMap: true,

  sourceMapEmbed: true,

  success: function(result) {
    console.log(result); // here result.css has embedded map
                         // and result.map has undesired source-map
  }
});

For now, what you guys can probably do in your success cb is:

function myCallback(result) {
  if(result.map && !myOptions.sourceMapEmbed) {
    // invoke IO op to write .map file
  }
  // do more with result obj
}
var myOptions = {
  success: myCallback,
  // other options
};
require('node-sass').render(myOptions);
simonexmachina commented 9 years ago

@am11, notwithstanding the node-sass issue you posted, I'm not aware of any issue here. This PR will write the source map to filesystem if the sourceMap option has been provided. What exactly are you suggesting is the issue here?

We could provide a workaround for the node-sass issue but I'd prefer to keep workarounds out of broccoli-sass and instead get the issue resolved in node-sass.

am11 commented 9 years ago

@aexmachina, this LibSass behaviour is a by design. Node-Sass is only a relay wrapper, it should not alter or supplement the decisions made by LibSass.

So in your code, you can probably consider changing:

if(this.sassOptions.sourceMap) {

to:

if(this.sassOptions.sourceMap && !this.sassOptions.sourceMapEmbed) {

And if you think about it, there is no decent work around for it in node-sass either; and doing something very strange like 'unsetting' the sourceMap option is a no-go..

simonexmachina commented 9 years ago

@am11, I do appreciate your input on this, but I'm afraid that your communication is quite unclear. Let me be very clear:

In the same way that node-sass should be a thin wrapper around libsass, broccoli-sass should be a thin wrapper around node-sass, and not introduce different semantics around options. The issue with sourceMapEmbed: true, sourceMap: false should really be a separate issue, and it's not one that I'd be willing to merge until we know whether this is going to be fixed in libsass.

am11 commented 9 years ago

@aexmachina, please specify which part is not clear to you? As of node-sass v2.0.1, you can use the aforementioned condition in broccoli-sass because in this PR you are writing the map file to file-system. The LibSass issue you are referring is a "proposal" to change the current behaviour that sourceMapEmbed option should not require sourceMap to be set.

simonexmachina commented 9 years ago
am11 commented 9 years ago

@aexmachina,

Are you suggesting changes to this PR?

Yes; because in this PR you added that condition in success callback.

If you're suggesting that we should change the semantics of

I am only suggesting to alter your condition like this.

this is going to be fixed in libsass

If they will address that issue, that will not be considered as a fix, but it will be a change in behaviour in future version of LibSass. The current behaviour is not 'flawed' per se but there is a room for improvement.

simonexmachina commented 9 years ago

Okay cool. The problem with your suggested change is that we would then have different semantics to libsass, so I'd like to wait to see what they want to do first.

Furthermore, I think that sourceMap: true should create a source map file on disk, and sourceMapEmbed: true should embed the source map in the CSS, and that they be separate and have no effect on each other ie. you shouldn't need to specify sourceMap: true in order to get an embedded source map. Your suggested change would not achieve this (you still need to specify sourceMap: true in order for sourceMapEmbed to work). Hence my desire to wait to see what libsass decide before implementing any changes regarding sourceMapEmbed.

am11 commented 9 years ago

Brilliant. :+1:

simonexmachina commented 9 years ago

Excellent! We made it :+1:

mphasize commented 9 years ago

:+1:

lessless commented 9 years ago

@joliss would you like to merge this, please?

mansona commented 9 years ago

Just for the record (in case you don't already know) I think @joliss is now working off a timebox where she dedicates a particular day a week to her Broccoli activities. I expect she will get around to this on her next allocated day so give it a week and check back :wink:

joliss commented 9 years ago

I'm closing this in favor of 6e600fed747516ea6c3c56758ae1215cd55330f7. Once we have the path handling kinks ironed out (https://github.com/sass/libsass/issues/908), we can hopefully bring back source map support.