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

Upgrade node-sass to v2 #41

Merged
merged 6 commits into from
Jan 17, 2015
Merged

Conversation

jstcki
Copy link
Contributor

@jstcki jstcki commented Jan 14, 2015

I ran into some issues with advanced SASS features, which could be fixed by upgrading node-sass to v2.0.0-beta. v2 also has some nice features like a list of included files which can directly be used to mark dependencies, and more accurate errors.

A couple of things I'm not really sure about:

  • Dependencies also have been marked when an error occurred. I removed this, since I don't think it's needed (and node-sass doesn't include a file list on error) but I couldn't really find any documentation for it.
  • I noticed the tests don't really seem to test the loader but just node-sass.

I've bumped the version to 0.4.0-beta.1, to indicate that we're still relying on the node-sass beta.

@jhnns
Copy link
Member

jhnns commented Jan 14, 2015

First of all, thank you for your pull-request. I'm glad you noticed the new includedFiles-feature, I've implemented it in node-sass 😀

We've discussed about marking dependencies. The problem is, that libsass is ignoring the whole module when there's a syntax error. Thus webpack's file watcher will not fire, even after you've fixed the error. This has probably been fixed with a new version of libsass (I recall a discussion about this at libsass). But before we're removing sass-graph we need to assert that all dependencies are marked (even those with a syntax error).

However, I'm willing to publish a beta of the sass-loader which will use the new node-sass version. There will also be custom importers which will integrate nicely with webpack's custom resolving engine.

@akiran
Copy link
Contributor

akiran commented Jan 15, 2015

We can remove sass-graph if libsass with this feature is supported by node-sass
sass/libsass#638.

I will test it and see.

@jstcki
Copy link
Contributor Author

jstcki commented Jan 15, 2015

Thanks @jhnns for the explanation. If I understand correctly it's possible to do this with libsass 3.1 but the list of files is not included on errors by node-sass yet?

I could revert the removal of sass-graph for the time being, so it would work as expected until node-sass supports this.

@jhnns
Copy link
Member

jhnns commented Jan 16, 2015

That would be great 👍

@jstcki
Copy link
Contributor Author

jstcki commented Jan 17, 2015

Done 😀

@JamesHenry
Copy link

Very excited for this!

jhnns added a commit that referenced this pull request Jan 17, 2015
@jhnns jhnns merged commit 51485cd into webpack-contrib:master Jan 17, 2015
@jhnns
Copy link
Member

jhnns commented Jan 17, 2015

I'm getting an error on node 0.11.14

module.js:355
  Module._extensions[extension](this, filename);
                               ^
Error: Module did not self-register.
    at Error (native)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jhnns/dev/jtangelder/sass-loader/node_modules/node-sass/lib/index.js:211:15)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)

This is probably related to a recent fix at node-sass. I don't think that we can publish the current state because it would break all projects using node 0.11.x.

@jhnns
Copy link
Member

jhnns commented Feb 3, 2015

We should probably also bump our major version, because according to node-sass it is a breaking change.

@jhnns
Copy link
Member

jhnns commented Feb 12, 2015

node-sass 2.0.0 has been released... sass/node-sass#157

@wolfeidau
Copy link

Is this going to be released?

Would be cool to not track master to use io.js

@jhnns
Copy link
Member

jhnns commented Feb 25, 2015

Has been published as 0.4.0

@wolfeidau
Copy link

👍 thanks a lot!

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.

5 participants