Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

add --require cli option #1617

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

add --require cli option #1617

wants to merge 7 commits into from

Conversation

xi
Copy link

@xi xi commented Jul 1, 2016

It is still quite confusing how to use eyeglass, and I guess part of the problem is that it is not supported by the node-sass cli.

You might argue that this does not belong here. I had first tried to create an eyeglass cli that would mirror node-sass. But that would be a lot of code duplication. The best argument against including this would be if there were other libraries like eyeglass. Obviously we can not add options for all of them. But as far as I know, there is only this one.

bin/node-sass Outdated
@@ -309,15 +316,15 @@ function run(options, emitter) {
emitter.emit('error', 'The --source-map option must be either a boolean or directory when compiling a directory');
}

if (options.importer) {
if (options.importer && typeof options.importer === 'string') {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not completely sure about this check. I had first used util.isString(), but that is deprecated. I also did not want to add additional dependencies like lodash.isstring.

@chriseppstein
Copy link
Contributor

I'd rather that node-sass have a public api that could allow projects like eyeglass to automatically load themselves if they conform to that api.

Tho, I guess that's basically the feature that eyeglass gives to node-sass so ¯_(ツ)_/¯

@xzyfer
Copy link
Contributor

xzyfer commented Jul 1, 2016

IMHO

It is still quite confusing how to use eyeglass

This is a problem with the eyeglass documentation. It focuses heavily on assets, integrations, and custom function but lacks a good getting started guide. This was something I'd previously hoped to resolve but my plate is very full.

it is not supported by the node-sass cli

I'm not sure these concerns overlap. It's not clear to me what to expect from an --eyeglass flag.

You might argue that this does not belong here

I don't believe this belong in node-sass.

if there were other libraries like eyeglass

I know there is at least sassport. I'm also aware of a handful of projects that integrate node-sass.

I had first tried to create an eyeglass cli

This is what I would have expected.

I'd rather that node-sass have a public api that could allow projects like eyeglass to automatically load themselves if they conform to that api.

I'm not sure what this would look like. We do lack something equivalent to Sass's --require option which might make sense.


We have previously discussed some kind of .sassrc file for configuring custom functions and importers. The idea has come up a few time but didn't get enough traction.

@xi
Copy link
Author

xi commented Jul 2, 2016

Thanks for the quick and extensive feedback!

After thinking about this again, I now believe that the focus is actually not that much on eyeglass, but on package managing: It feels as eyeglass could be to node-sass what npm is to node. In that regard it is also different from sassport.

We have previously discussed some kind of .sassrc file for configuring custom functions and importers.

I thought about this and it seemed like a good idea at first, but then I realised it would be like re-implementing grunt. I really have no intention of turning node-sass into a fully featured build tool.

I guess the problem is really that node-sass has no build-in support for package managing. Eyeglass solves that problem, but it does not work with standard node-sass cli.

I am not sure what to make of this yet. I will try to look into the autoloading idea mentioned by chriseppstein.

@xi
Copy link
Author

xi commented Jul 2, 2016

Just a quick idea to hear what you think of this: What if the option was not called --eyeglass but --npm?

Tobias Bengfort added 2 commits July 2, 2016 10:29
@xi
Copy link
Author

xi commented Jul 2, 2016

We do lack something equivalent to Sass's --require option which might make sense.

I tried this one now. node-sass --require eyeglass should do the same as node-sass --eyeglass did before.

Tobias Bengfort added 2 commits July 7, 2016 10:57
@xi xi changed the title add eyeglass cli option add --require cli option Sep 12, 2016
@xi
Copy link
Author

xi commented Sep 12, 2016

I did not get any feedback for two month now. Is there anything I can do to get this merged?

@saper
Copy link
Member

saper commented May 7, 2017

Sorry for keeping you waiting @xi

I remember I was also very confused trying to start with eyeglass. Can you give me short a short example how things get easier? Maybe we could add it the docs.

@xi
Copy link
Author

xi commented May 8, 2017

Running node-sass --require eyeglass style.scss will use eyeglass to resolve dependencies. But this could also be used as a hook for other tools.

@xi
Copy link
Author

xi commented May 25, 2017

I am not sure why appveyor fails. Looks unrelated to me.

@nschonni nschonni requested review from kevva and removed request for kevva June 14, 2017 05:20
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Change C-API to pass compiler to custom functions
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants