-
-
Notifications
You must be signed in to change notification settings - Fork 430
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
Allow node-sass
importers to be passed in options.
#170
Conversation
// `node-sass`'s importer takes an array of functions. Since `parseQuery` doesn't allow functions, | ||
// `opt.importer` has to be a path or array of paths that, once `require`d, return importer functions. | ||
opt.importer = [] | ||
.concat(opt.importer || []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could easier been written like:
opt.importer = (opt.importer || [])
.concat(getWebpackImporter())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opt.importer
may be a string (to mirror node-sass
s importer
field which takes a function or array of functions).
Thx for your PR 👍 I'm definitely planing to merge this, but it would be good if we'd follow the official recommendation for programmable objects in query parameters. The sass-loader should not try to This is also related to #152 |
@jhnns Sounds good, thanks for the link (I'm pretty new to Webpack land). I'll make the update soon |
@jhnns Updated per the docs. Let me know what you think. |
👍 for this! |
Allow `node-sass` importers to be passed in options.
Shipped with |
Awesome, thanks! |
Thanks for the work on this. I'm trying to get this set up with node-sass-json-importer and am running into an issue. I'm wondering if someone here can help me out. Here's the configuration I added to my webpack config: sassLoader: {
importer: require('node-sass-json-importer'),
} With this configuration sass-loader seems to have a hard time resolving my
I found the bit of code where this is managed in sass-loader and it seems to add the webpack importer so I'm not really sure why this is failing like this. Any thoughts? |
I seem to have the same results with the following configuration, which from what I can tell shouldn't really do much of anything: sassLoader: {
importer: (url) => ({ file: url })
}, I also tried this, which seemed to work fine: sassLoader: {
importer: (_, __, done) => done()
}, Do you suppose there might be something else that's odd about my config? |
Acccording to the node-sass docs, you should return |
If you don't return |
Ah, thanks! It seems that this might be a bug in node-sass-json-importer then (https://github.com/Updater/node-sass-json-importer/blob/f203f2244d157ccce88194d31740d3b54e39128f/src/index.js#L24-L26). I'll move the discussion over there. |
According to the node-sass documentation: > If an importer does not want to handle a particular path, it should > return `sass.NULL`. https://github.com/sass/node-sass#importer--v200---experimental This will allow other importers to properly handle the import. Some more discussion on this can be found at: webpack-contrib/sass-loader#170 (comment) Since the documentation says that this is true as of 3.0.0, that's where I set the dependency version.
According to the node-sass documentation: > If an importer does not want to handle a particular path, it should > return `sass.NULL`. https://github.com/sass/node-sass#importer--v200---experimental This will allow other importers to properly handle the import. Some more discussion on this can be found at: webpack-contrib/sass-loader#170 (comment) Since the documentation says that this is true as of 3.0.0, that's where I set the dependency version.
This allows for custom importers to be passed in the loader query.
For example, node-sass-json-importer could be enabled like so:
The above example also resolves #111.
Was hoping to write a test for this, but it seems a bit difficult to fit into the well structured but somewhat rigid test setup. Let me know what you think.