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

Sass options are not passed through. #57

Open
chriseppstein opened this issue Mar 31, 2015 · 8 comments · May be fixed by #71
Open

Sass options are not passed through. #57

chriseppstein opened this issue Mar 31, 2015 · 8 comments · May be fixed by #71

Comments

@chriseppstein
Copy link

https://github.com/joliss/broccoli-sass/blob/master/index.js#L26-L34

Only some sass options are passed through. Each node-sass release can introduce new options and users can provide their own custom options to their custom functions (new feature in node-sass 3.0), so instead, the user's options should be cloned and then modified/augmented for the specific options that broccoli-sass cares about changing.

If you agree this is a good change, I will work on a patch.

@ieugen
Copy link

ieugen commented Apr 1, 2015

+1

@joliss
Copy link
Owner

joliss commented Apr 1, 2015

I remember regretting doing option pass-through in this vein in the past (with other plugins), so in general I prefer to keep it explicit and just update it when node-sass gains new options. On the other hand,

users can provide their own custom options to their custom functions

this might be a reason to do passthrough. I'm unsure.

So for example, at the moment, we intentionally don't support the source map options, but once we do, the plan is to make it "just work", so that file paths in source maps are wired up correctly. Some of the options might not make sense in the Broccoli ecosystem, so for some options we actually don't want to passthrough.

Anyways, I'm unsure.

@chriseppstein
Copy link
Author

we intentionally don't support the source map options

Ignoring the source map option silently is a really confusing behavior. If you've decided that some option can't be supported, that should be an explicit error or warning.

Some of the options might not make sense in the Broccoli ecosystem

What are the class of options where this would be the case?

It seems to me that when those are found, they should also be documented and turned into errors/warnings.

I'm unsure.

This is blocking eyeglass from building on broccoli-sass for our integration with ember. If pass-through was the default, then there would be no issue. I think the concerns of things not working will be the exception and should be handled as issues when it happens, by and large, the options are going to be safe to pass along.

Either way, at the very least, I need some escape valve where users can customize the options that pass through to node-sass.

@simonexmachina
Copy link
Collaborator

FWIW I'm an advocate of passing through options to node-sass as-is (omitting any that are specifically for broccoli-sass). I think we can deal with any issues with specific conflicts that may arise on their merits.

@jamesmenera
Copy link

Any workarounds in the meantime?

@chriseppstein
Copy link
Author

@joliss actually, my requirements ended up diverging to the point where using broccoli-sass didn't make sense. Instead, I have written a broccoli sass compiler for use by eyeglass. Feel free to close this issue.

@barneycarroll
Copy link

This is a common issue with middleware. I've been using various forms of source transformation recently, and the need to pass options to another plugin is quite common. To a certain degree, it's possible to use subargs: https://github.com/substack/subarg — the logic can be extended to a JS config object, which is to say that the arguments to ignore and pass on are defined in a nested object with a name reflective of that purpose.

Of course when your configuration becomes quite esoteric it's unhelpful to have it hidden away in some build process' imperative initialization script (ie why is my SASS screwing up? Check out my Ember config, which invokes Broccoli, which invokes LibSASS, etc). At this point it helps to have a config file that that process can read from regardless of where it's been invoked, so you can separate your concerns. And sadly, it seems the only reference to config files for SASS refers back to .rb files for RubySASS. Maybe that's the place to push for that, rather than getting every piece of middleware to account for evolving SASS config requirements in their own fashion?

@sandstrom
Copy link

Passing settings through would be very useful! Ideally a single hash, but if you'd really want to keep them apart there can be two option hashes, one for broccoli-sass, and one passed through to the underlying library.

@sandstrom sandstrom linked a pull request Jul 26, 2016 that will close this issue
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 a pull request may close this issue.

7 participants