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

Allow node-sass importers to be passed in options. #170

Merged
merged 1 commit into from
Oct 23, 2015

Conversation

pmowrer
Copy link
Contributor

@pmowrer pmowrer commented Sep 30, 2015

This allows for custom importers to be passed in the loader query.

For example, node-sass-json-importer could be enabled like so:

{
  test: /\.scss$/,
  loaders: [
    "style", 
    "css", 
    "sass?" + JSON.stringify({
      importer: require.resolve('node-sass-json-importer')
    });
  ]
}

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.

// `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 || [])
Copy link
Member

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())

Copy link
Contributor Author

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-sasss importer field which takes a function or array of functions).

@jhnns
Copy link
Member

jhnns commented Oct 5, 2015

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 require() the file, but just read it from the webpack config.

This is also related to #152

@pmowrer
Copy link
Contributor Author

pmowrer commented Oct 5, 2015

@jhnns Sounds good, thanks for the link (I'm pretty new to Webpack land). I'll make the update soon

@pmowrer
Copy link
Contributor Author

pmowrer commented Oct 7, 2015

@jhnns Updated per the docs. Let me know what you think.

@lukasgeiter
Copy link

👍 for this!

jhnns added a commit that referenced this pull request Oct 23, 2015
Allow `node-sass` importers to be passed in options.
@jhnns jhnns merged commit 055c88e into webpack-contrib:master Oct 23, 2015
@jhnns
Copy link
Member

jhnns commented Oct 25, 2015

Shipped with 3.1.0. New preferred way for passing options is now via a property on your webpack config. See https://github.com/jtangelder/sass-loader#sass-options

@lukasgeiter
Copy link

Awesome, thanks!

@lencioni
Copy link
Contributor

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 @imports that use ~ and I get messages like the following:

ERROR in ./~/css-loader?localIdentName=[path][name]--[local]--[hash:base64:10]!./~/autoprefixer-loader!./~/sass-loader!./app/assets/javascripts/components_loader.js!./app/assets/components/MyComponent/index.scss
Module build failed:
@import '~stylesheets/config';
^
      File to import not found or unreadable: ~stylesheets/config
Parent style sheet: stdin
      in /Users/me/repo/app/assets/components/MyComponent/index.scss (line 1, column 1)
 @ ./app/assets/components/MyComponent/index.scss 4:14-286

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?

@lencioni
Copy link
Contributor

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?

@jhnns
Copy link
Member

jhnns commented Nov 30, 2015

Acccording to the node-sass docs, you should return sass.NULL if your custom importer doesn't want to handle the file. This passes the file request on to the next importer in the array.

@jhnns
Copy link
Member

jhnns commented Nov 30, 2015

If you don't return sass.NULL in cases you don't want to resolve the file, all webpack specific resolving won't be applied.

@lencioni
Copy link
Contributor

lencioni commented Dec 1, 2015

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.

lencioni added a commit to lencioni/node-sass-json-importer that referenced this pull request Dec 1, 2015
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.
lencioni added a commit to lencioni/node-sass-json-importer that referenced this pull request Dec 1, 2015
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.
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.

Loading JSON files with sass-loader
4 participants