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

functions option support #152

Merged
merged 1 commit into from
Oct 5, 2015
Merged

Conversation

sanniassin
Copy link

Since version 3.0.0 node-sass supports custom functions, so it's gonna be great to use it with sass-loader

@sanniassin
Copy link
Author

Makes sense, but not sure about conflicts if same option present both in query and sassLoader.

@jorrit
Copy link
Contributor

jorrit commented Aug 23, 2015

I'd say that sassLoader overwrites opts. The object-assign module could be used for this.

@sanniassin
Copy link
Author

I mean, that two points for setting same options could lead to undesirable behavior with hard to find overrides if you load options for sassLoader from external module or just forgot, that option already set in sassLoader. Anyway it's better to make another PR for this.

@dtothefp
Copy link

This may be a moot point because from what I can tell the loader-utils module won't even allow the user to pass an object and have it parsed. I've tried many different methods with no luck webpack/loader-utils#15.

eslint-loader uses the object-assign pattern where the options passed from wepback config task presedence, i.e. matching keys passed from the query string are overwritten by those present in this.options. https://github.com/MoOx/eslint-loader/blob/master/index.js#L98 Although eslint-loader shows in the readme a way to pass rules as object loader: "eslint-loader?{rules:[{semi:0}]}" in the query string, I tried it out in sass-loader and didn't get it to work so not sure that the loader-utils API even supports this?

screen shot 2015-08-31 at 8 52 44 am

screen shot 2015-08-31 at 8 52 49 am

Update: got passing object as query param to work...duh

screen shot 2015-08-31 at 9 02 29 am

I do agree with @jorrit that all keys should be merged, but I would say that those in this.options take precedence over those from parsing the query string as they are less prone to errors. It would also be great to have this behavior because it is currently a pain to iterate over all includePaths and create a query param for each.

Anyhow, it would be great to have this functionality. Not sure if @jhnns would be open to having a dependency on object-assign added? Ultimately, I think there is a problem with @sokra loader-utils API if for somethings such as Objects a different API for passing options i.e. this.options vs parsing this.query to loaders must be used.

@jhnns
Copy link
Member

jhnns commented Sep 11, 2015

I had this issue several times (webpack-contrib/less-loader#40 peerigon/markdown-loader#8). While passing the options via query parameters is cumbersome, I don't think that it would be a good idea to pass-in all options via the webpack config. It seems straight-forward, but the full module path + query string is used as cache id afaik. Hence, two require calls with different loader configs are recognized to be different.

What do you think about it @sokra?

@jhnns
Copy link
Member

jhnns commented Oct 5, 2015

@sanniassin anyway, thx for your PR. I'm willing to merge this, but could you also add tests please?

@jhnns
Copy link
Member

jhnns commented Oct 5, 2015

Nevermind... I'll do it, because we need a better setup for these kind of tests.

jhnns added a commit that referenced this pull request Oct 5, 2015
@jhnns jhnns merged commit a6cc46f into webpack-contrib:master Oct 5, 2015
@sanniassin
Copy link
Author

Thanks, as i've never used mocha before :)

@dtothefp
Copy link

dtothefp commented Oct 6, 2015

@jhnns thanks for the help on this. I know it sucks for caching but seems like for complex config this is necessary in webpack

@jhnns
Copy link
Member

jhnns commented Oct 9, 2015

@dtothefp It is necessary for options that can not be stringified using JSON.stringify

@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

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.

4 participants