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

simonexmachina · 2015-01-13T10:29:49Z

No description provided.

ryanto · 2015-01-30T20:09:52Z

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: sass/node-sass#517

simonexmachina · 2015-01-30T23:42:59Z

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

Globegitter · 2015-02-04T15:58:27Z

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

simonexmachina · 2015-02-05T21:43:09Z

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

joliss · 2015-02-05T22:04:12Z

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 · 2015-02-05T22:10:44Z

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 · 2015-02-05T23:24:44Z

Okay that's fixed now @joliss.

Update version

fivetanley · 2015-02-12T04:51:32Z

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

simonexmachina · 2015-02-12T05:11:00Z

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 · 2015-02-12T22:10:17Z

@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 · 2015-02-13T17:39:04Z

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 · 2015-02-13T21:12:48Z

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

simonexmachina · 2015-02-13T22:05:54Z

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
#43 (comment).

am11 · 2015-02-15T05:01:28Z

@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 sass/libsass#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 · 2015-02-16T06:58:05Z

@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 · 2015-02-16T10:49:38Z

@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 · 2015-02-16T22:00:25Z

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

I was referring to the libsass issue you posted, not node-sass (my bad)
You are correct that node-sass should just pass options through to libsass, and the same is true of broccoli-sass
I think that libsass should embed the source map if sourceMapEmbed: true, sourceMap: false, but this is their decision (I think this is what you're saying?). That's why I added the comment to API: source-map related options enabling criteria sass/libsass#885 seeking clarification on their intentions with this.

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 · 2015-02-16T22:32:59Z

@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 · 2015-02-16T22:39:56Z

Are you suggesting changes to this PR?
If you're suggesting that we should change the semantics of sourceMapEmbed: true, sourceMap: false then I've responded to that above ("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 · 2015-02-16T22:56:08Z

@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 · 2015-02-16T23:01:19Z

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 · 2015-02-16T23:05:21Z

Brilliant. 👍

simonexmachina · 2015-02-16T23:05:54Z

Excellent! We made it 👍

mphasize · 2015-02-18T14:18:58Z

👍

lessless · 2015-02-18T14:35:06Z

@joliss would you like to merge this, please?

mansona · 2015-02-18T18:04:45Z

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 😉

Temporary fix due to using node v0.12. Once joliss/broccoli-sass#43 has been merged, this can be removed.

joliss · 2015-02-27T01:37:11Z

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

Added support for sourcesContent and update to node-sass 2.0.0-beta

f58d14f

simonexmachina added 2 commits February 6, 2015 10:27

Added support for sourcesContent and update to node-sass 2.0.0-beta

37c5c9a

Update version

Only produce the .map file if sourceMap is true

7a96141

simonexmachina mentioned this pull request Feb 12, 2015

Update node sass to 2.0.1 #45

Closed

Update to node-sass@2.0.1

05afee0

simonexmachina mentioned this pull request Feb 12, 2015

can't install on iojs 1.1.0 - 1.2.0 #46

Closed

davidwallacejackson mentioned this pull request Feb 13, 2015

update dependencies to allow node-sass@2.0.0 #47

Closed

simonexmachina mentioned this pull request Feb 13, 2015

Issue installing after running npm install #48

Closed

Globegitter mentioned this pull request Feb 15, 2015

'ember g ember-cli-foundation-sass' not working on io.js 1.2.0 nadavshatz/ember-cli-foundation-sass#20

Closed

am11 mentioned this pull request Feb 15, 2015

API: source-map related options enabling criteria sass/libsass#885

Closed

denniszhao mentioned this pull request Feb 22, 2015

Cannot install with ember-cli 0.2.0-beta.1 #50

Closed

eviltrout mentioned this pull request Feb 23, 2015

npm install fails with node 0.12 eviltrout/ember-performance#37

Closed

simonexmachina changed the title ~~Added support for sourcesContent and update to node-sass 2.0.0-beta~~ Added support for sourcesContent and update to node-sass v2.0.1 Feb 23, 2015

green-arrow pushed a commit to green-arrow/ember-cli-chosen that referenced this pull request Feb 26, 2015

Temporarily adding broccoli-sass fork.

a7bda1f

Temporary fix due to using node v0.12. Once joliss/broccoli-sass#43 has been merged, this can be removed.

joliss closed this Feb 27, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

simonexmachina commented Jan 13, 2015

ryanto commented Jan 30, 2015

simonexmachina commented Jan 30, 2015

Globegitter commented Feb 4, 2015

simonexmachina commented Feb 5, 2015

joliss commented Feb 5, 2015

simonexmachina commented Feb 5, 2015

simonexmachina commented Feb 5, 2015

fivetanley commented Feb 12, 2015

simonexmachina commented Feb 12, 2015

simonexmachina commented Feb 12, 2015

Ghostavio commented Feb 13, 2015

Globegitter commented Feb 13, 2015

simonexmachina commented Feb 13, 2015

am11 commented Feb 15, 2015

simonexmachina commented Feb 16, 2015

am11 commented Feb 16, 2015

simonexmachina commented Feb 16, 2015

am11 commented Feb 16, 2015

simonexmachina commented Feb 16, 2015

am11 commented Feb 16, 2015

simonexmachina commented Feb 16, 2015

am11 commented Feb 16, 2015

simonexmachina commented Feb 16, 2015

mphasize commented Feb 18, 2015

lessless commented Feb 18, 2015

mansona commented Feb 18, 2015

joliss commented Feb 27, 2015

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

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

Conversation

simonexmachina commented Jan 13, 2015

ryanto commented Jan 30, 2015

simonexmachina commented Jan 30, 2015

Globegitter commented Feb 4, 2015

simonexmachina commented Feb 5, 2015

joliss commented Feb 5, 2015

simonexmachina commented Feb 5, 2015

simonexmachina commented Feb 5, 2015

fivetanley commented Feb 12, 2015

simonexmachina commented Feb 12, 2015

simonexmachina commented Feb 12, 2015

Ghostavio commented Feb 13, 2015

Globegitter commented Feb 13, 2015

simonexmachina commented Feb 13, 2015

am11 commented Feb 15, 2015

simonexmachina commented Feb 16, 2015

am11 commented Feb 16, 2015

simonexmachina commented Feb 16, 2015

am11 commented Feb 16, 2015

simonexmachina commented Feb 16, 2015

am11 commented Feb 16, 2015

simonexmachina commented Feb 16, 2015

am11 commented Feb 16, 2015

simonexmachina commented Feb 16, 2015

mphasize commented Feb 18, 2015

lessless commented Feb 18, 2015

mansona commented Feb 18, 2015

joliss commented Feb 27, 2015