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

Use proper hook for SASS loader. #53

Closed

Conversation

izaakschroeder
Copy link

This changes a lot of stuff and the tests need to be updated to match. This builds a small project I have going right now but I haven't tested it majorly on anything else. Someone should look into it.

@izaakschroeder
Copy link
Author

This is a good start on #31 and #50.

This changes  a lot of stuff and the tests need to be updated to match. This builds a small project I have going right now but I haven't tested it majorly on anything else. Someone should look into it.
@saki7
Copy link

saki7 commented Mar 4, 2015

👍

@jhnns
Copy link
Member

jhnns commented Mar 5, 2015

Sorry for not answering this.

First of all: Thank you for work. I appreciate every contribution. Unfortunately this PR looks a bit over-complicated to me. Imho you're using too much internal stuff which can then break easily when @sokra changes something.

We can take the less-loader as blueprint since it already has this feature for quite some time. The important code is here. We can let webpack/enhanced-resolve do the resolving by calling

// loaderContext is `this` inside the loader
loaderContext.resolve(context, moduleRequest, cb)

Take also a look at the loader api.

I'll publish a branch which contains the refactoring for node-sass@2.0.0

@izaakschroeder
Copy link
Author

@jhnns A lot of this was indeed lifted from less-loader. However, one thing less-loader is missing (and this is not) is the ability to use module urls in imports.

@import "foo!/something";

The above is otherwise not possible.

Also, loaderContext.resolve has its own set of problems which I struggled at before arriving at the current solution (in particular only one context is allowed and imports require more than one: root directory + "current directory" which is exposed really poorly in node-sass). The custom resolver is pretty much ripped from the webpack config loader.

If you don't want to merge this please close the branch and I'll maintain a fork myself, as I need the module loading capability.

@jhnns
Copy link
Member

jhnns commented Mar 5, 2015

Oh, ok... didn't know that you already tried the less-loader approach.

@izaakschroeder what do you mean by "module urls"? Inlined loaders?
@sokra is that true? Does loaderContext.resolve not apply inline loaders?

I don't know what you mean by "root directory", but the described behavior should be done with loaderUtils.urlToRequest(url)

@izaakschroeder
Copy link
Author

@jhnns the module loader allows you to do stupid things like @import "css!less!foo"; where you can get a module to go through a precompile phase before being passed into node-sass. I use it for generating icon fonts, though I have not yet published the module. I could have sworn I tried every other approach, but they don't accept the "foo!bar" style.

How do you specify multiple paths to be searched? This approach allows you to feed the resolver multiple base paths and it will go through them in order, since SASS convention has no concept of "./foo" vs "foo" both have to be tried.

@jhnns
Copy link
Member

jhnns commented Mar 5, 2015

Check out this chapter of loader-utils.

The ~-prefix points to module directories. This should be consistent across all loaders where the import/require syntax doesn't differentiate between local and module directories. The css-loader and less-loader are already using it.

@grrowl grrowl mentioned this pull request Mar 16, 2015
@jhnns
Copy link
Member

jhnns commented Mar 22, 2015

I've implemented the custom importer with the less-loader approach.

Anyway, thanks for your suggestions 👍

@jhnns jhnns closed this Mar 22, 2015
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.

3 participants