Implement support for multiple importers

[mgreter]

This PR now implements all options I envisioned in #962. At this point I'm not sure if we want to include this in 3.2.0 or wait for 3.2.1, since it does bring a few breaking changes with custom importers and custom functions (not that many, but still noticeable). @am11 what do you think, do you have time to adjust this in node-sass before 3.0? It basically just involves setting a list of importers instead of a single one (plus passing an additional priority value on creation). Other than that everything can stay as it is.

How do custom importers work now?

For each @import libsass will call importers in priority order (highest priority first). If the importer returns 0, libsass will skip to the next one. If no importer returns anything, libsass will continue to load the import on its own. Once an importer returns something (either a list of imports, an empty list of imports or an error), the chain is aborted and only the given return is handled. Additionally I fixed the behaviour when only a filename is returned to be exactly as with plain libsass imports.

Breaking C-API changes

To create custom importer list:

double priority = 0; // default
void* cookie = (void*) pointer_to_anything;
Sass_Importer_List importers = sass_make_importer_list(1);
importers[0] = sass_make_importer(sass_importer, priority, cookie);

New function signatures for custom functions and importers:

Sass_Import_List sass_importer(const char* cur_path, Sass_Importer_Entry cb, struct Sass_Compiler* comp)
union Sass_Value* call_sass_function(const union Sass_Value* s_args, Sass_Function_Entry cb, struct Sass_Options* opts)

The options that have previously been passed can now be fetched indirectly:

void* cookie = sass_importer_get_cookie(cb);
struct Sass_Import* previous = sass_compiler_get_last_import(comp);
const char* prev_path = sass_import_get_path(previous);
void* cookie = sass_function_get_cookie(cb);

You should now be able to access all context options via API calls.

struct Sass_Context* ctx = sass_compiler_get_context(comp);
struct Sass_Options* opts = sass_context_get_options(ctx);
double precision = sass_option_get_precision(opts);

Renamed a lot of typedefs regardings function and importer API. They should now make much more sense. Not sure how big of an impact this has on implementers though, but it should make it a lot more future-proof. Signatures for functions and importers should not need to change again, since they can now access all options available indirectly via the passed arguments.

// Typedef helpers for import lists
typedef struct Sass_Import (*Sass_Import_Entry);
typedef struct Sass_Import* (*Sass_Import_List);
// Typedef helpers for custom importer lists
typedef struct Sass_Importer (*Sass_Importer_Entry);
typedef struct Sass_Importer* (*Sass_Importer_List);
// Typedef defining importer signature and return type
typedef Sass_Import_List (*Sass_Importer_Fn)
  (const char* url, Sass_Importer_Entry cb, struct Sass_Compiler* compiler);

// Typedef helpers for custom functions lists
typedef struct Sass_Function (*Sass_Function_Entry);
typedef struct Sass_Function* (*Sass_Function_List);
// Typedef defining function signature and return type
typedef union Sass_Value* (*Sass_Function_Fn)
  (const union Sass_Value*, Sass_Function_Entry cb, struct Sass_Options* options);

Renamed structs/typedefs (probably incomplete):

Sass_C_Function -> Sass_Function_Fn
Sass_C_Function_List -> Sass_Function_List
Sass_C_Function_Callback -> Sass_Function_Entry
Sass_C_Function_Descriptor -> Sass_Function
Sass_C_Import_Fn -> Sass_Importer_Fn
struct Sass_Importer** -> Sass_Importer_List
Sass_C_Import_Callback -> Sass_Importer_Entry
Sass_C_Import_Descriptor -> Sass_Importer
struct Sass_Import** -> Sass_Import_List
struct Sass_Import* -> Sass_Import_Entry

New experimental feature

Custom headers (#960) act like importers, but only run once at the beginning of each compilation. They are primarily meant to define custom mixins (via plugins or context options). Beside that they might be usefull for other cool things I currently cannot think of 😄 Opposite to custom importers, all custom headers will be executed in priority order and all imports will be accumulated (so many custom headers can add various custom mixins or css-code).

Include in 3.2.0 !?

From a strict versioning stand-point, this should be included in 3.2 since it contains quite a few breaking changes in the API. The new plugins API would also benefit from it. Adding it to 3.2.1 seems wrong and since it is only stabilizing experimental features, I re-scope this to 3.2 (open for further discussions). I touches quite some code though to be added that late in the beta phase. IMO we should do at least another beta round to let node-sass catch-up to these changes.

//CC @xzyfer @am11 @chriseppstein

[drewwells]

Possibly related. Is it possible to get a list of files used by the file compiler? This would be super handy for implementor building a file watcher. That rebuilds correctly based off imported file changes.

[mgreter]

@drewwells you are probably looking for included_files, which is already documented in the wiki!

[am11]

This looks quite interesting @mgreter. 👍

I will try to wrap my head around how to provision the priority thing in JS and/or decent way to capture the intent of user that which kind of import they want to handle with certain importer, so to translate that in API call.

Currently, I am under the impression that we will allow user to make their decision and return null; whether certain importer wants to handle certain kind of URI? In which case, the user will return null or undefined from JS code, we will return nullptr to libsass and libsass will call the next importer in pipe. If there is no next-importer in queue, libsass will fallback to default mechanism.

Please CMIIW.

+1 to add it in v3.2. We already have fistful of breaking changes coming in node-sass v3, why not another! 😎

[mgreter]

@am11 yup, spot on. Also check out custom headers, which are registered exactly the same way as importers. Only difference is that all custom headers will be called and their imports get accumulated. It's really just an array of importers that get called at the start of each compilation. As for the priority, you may be able to attach a priority property to the js function? Like so:

function importer(...) {
  return ['filename', 'div { ... }'];
}
importer.priority = 99;
var options = {
  // old and obsolete
  // importer => importer,
  importers => [
    importer, // = importer.priority || 0
    [ importer, 5 ] // = 5 || importer.priority
  ]
}

In perl I opted for a simple array ref:

headers => [
  header1, // will get default = 0
  [ header2, 1] // gets specific prio
]

What priorities people give to their importers is pretty much out of this scope, but I could propose these very personal guidelines, which are probably really just a bad first guess:

>0 : headers that only define stuff
<=0 : headers that output css code
1 - 100 : for personal stuff
100 - 1000 : for framework stuff
1000 + : for distributed frameworks

[am11]

This is brilliant! ⚡

I think we can let node-oriented users return the priority as member of object, instead of doing something fancy/tricky like this.priority=.., they will just return {file: ..., contents: ..., priority: ..} (or the async variant done({file: .., contents: ..., priority: ...})).

As for the custom headers, I will try to add it to v3, but I am not sure if we would have enough time after this gets merged and libsass v3.2.0 stable is released? Will there be another beta or rc, so we have time to include these features and be able to align node-sass v3.0 release with libsass 3.2?

[mgreter]

Be aware that the priority has to be passed to libsass before the importer gets called (you cannot include it in the return of the importer). And yes, I could merge this PR and create another beta, so node-sass can catch up and do another beta on its own, so things can get settled! IMO you should be able to copy pretty much all code from custom importers for custom headers.

[am11]

has to be passed to libsass before the importer gets called

I just envisioned it how it will look, and let me tell you this much: this is going to be fun 😎 ☀️

We will need to provide the following variants:

• importer: function
  • the priority 0 will be sent to libsass from node-sass binding
• importer: { priority, function }
  • the user-defined priority will be sent to libsass
• importer: [function1, function2, ..]
  • the array index will be sent as priority
• importer: [{priority, function}, {priority, function}, ..]
  • the user-defined priority will be sent.
• importer: [{priority, function}, {priority, function}, {function /* no priority here! */}..]
  • the user-defined priority will be sent, and the index of array for the missing priority.
  • not sure what will happen if there is a tie in priorities (user-defined vs. index-decuded)

Same goes for the headers.

Aside: we really need help from someone to document all that libsass via node-sass has to offer to node.js paradigm.

[am11]

After all, this is issue #:100:0 !

[coveralls]

Coverage decreased (-0.35%) to 80.07% when pulling f98127a on mgreter:feature/expose-file-resolver into ba0b3e0 on sass:master.

[drewwells]

I'm getting cannot find errors on sass_importer_set_list_entry. I'm kinda dumb, so I grepped to find answers.

sass_make_importer_list has an entry in sass_functions.cpp, but sass_importer_set_list_entry does not. Is that why it's not working?

[mgreter]

It looks like it was renamed to sass_import_set_list_entry ...

[mgreter]

Maybe helpful: commit needed for perl-libsass to bring it on par with 3.2.0-beta.4!

[drewwells]

That is helpful! The code indexes the C array directly rather than use a sass_importer_set_entry (which seems to be different than sass_import_set_entry): sass/perl-libsass@85fbc05#diff-5b390435e30d16e259818ad6b65d1dafR576

[chriseppstein]

I am opposed to the custom headers feature. One of the philosophies that Sass has is that we do not support things "magically being available". Only explicit declarations can and should bring in code dependencies. This is why there's no way to set a global variable from the sass command line despite numerous requests. Custom importers are a great way to bring in code that is constructed on the fly. That is how compass sprites worked. They imported the sprite images. IMO, we don't need a new abstraction here, the custom importer concept is enough.

[chriseppstein]

Load paths is just a simple way to make a filesystem importer. In this way, importers are actually the first class citizens and it allows importers and filesystem paths to be intermixed. Furthermore, the resolution order should just be a list IMO. I don't understand why we need priority.

[drewwells]

Custom functions is the new way of supporting the import overrides in Ruby
Sass. These work very well and less confusing in my opinion. From code I've
seen, it was quite rare for anybody to actually understand the various code
imports Compass supported. They just @import "compass" and hope for the
best. I think the explicit overriding of functions is a far better way to
support these mechanisms.

Since libsass has no custom mixin support, headers fill that gap quite
nicely. I understand your hesitation about custom headers. It does place
magic into the end user code that may have undesirable affects. See the
discussion on #489. True people could
put code in there, but it's not a developer detail it's an implementor
detail. Removing custom headers wouldn't prevent implementors doing it
another way like I've had to for a while now. I am quite happy to now have
custom headers to do this with.

On Wed, Apr 1, 2015 at 5:17 PM Chris Eppstein [email protected]
wrote:

Load paths is just a simple way to make a filesystem importer. In this
way, importers are actually the first class citizens and it allows
importers and filesystem paths to be intermixed. Furthermore, the
resolution order should just be a list IMO. I don't understand why we need
priority.

—
Reply to this email directly or view it on GitHub
#1000 (comment).

[mgreter]

We have two ways to add custom functions, headers and importers in libsass. Via the C-API options for implementers or by loading native libsass plugins. So the priority is needed since we have more than one array to consider. From the libsass implementation it's just a slightly different use of custom importers, so the feature was quite cheap and I do see the benefits in regard to native plugins.

[chriseppstein]

I get that custom functions violate the "magically available" principle that I mentioned. As such, I've been proposing that we move away from that pattern (sass/sass#1608 (comment)) so that even functions have to be loaded explicitly to be available in a sass file's compile.

I get that you find it useful, but it's moving libsass in a direction that sass itself is moving away from. I don't understand why you think libsass needs "custom mixins". Mixins come from imports. If you want to provide a mixin, you either provide a sass file or you provide a custom importer. The reason we need custom functions is to do things that pure sass cannot do, like filesystem access, or integration with build systems, etc. These functions can return whatever information the mixins need.

[chriseppstein]

@mgreter OK, I get that native plugins have this issue, but the priority value is really unintuitive to me. There's no agreement what any priority value means and any scheme that may arise would likely be better implemented in a less granular way. IMO, the priority from native plugins should be limited to HIGH and LOW where high priority plugins are guaranteed to be considered before the load path and low priority plugins are guaranteed to be considered after the load path. Then, any further resolution should belong to the implementer's API.

[drewwells]

Loading specific functions would be rather tedious. Maybe packages that
define sets of functions would be an affective way of making functionality
available. The imports are a good idea for a library of functionality. The
only issue I see is that errors would not give any indication of what went
wrong. Function not available couldn't be tracked back to a missing package
as libsass would no idea what package the function comes from.

Thinking of this problem like most modern language. IDEs provide support
for manipulating the import selection based on the code. This isn't
something Sass users can lean on. Calling "package-function" would add the
necessary compass import. It would actually be easier to track these things
down if rather than @function {function name} was referenced from a package
@function {package}.{function name}. This connects the functions back to
packages and relies less on the 'just works' principle that custom headers
and custom functions provides.

On Wed, Apr 1, 2015 at 6:18 PM Chris Eppstein [email protected]
wrote:

@mgreter https://github.com/mgreter OK, I get that native plugins have
this issue, but the priority value is really unintuitive to me. There's no
agreement what any priority value means and any scheme that may arise would
likely be better implemented in a less granular way. IMO, the priority from
native plugins should be limited to HIGH and LOW where high priority
plugins are guaranteed to be considered before the load path and low
priority plugins are guaranteed to be considered after the load path. Then,
any further resolution should belong to the implementer's API.

—
Reply to this email directly or view it on GitHub
#1000 (comment).

[mgreter]

@chriseppstein I agree that it is probably over engineered to expose the priority to the "options" C-API (which is what implementers use). From our point it is just convenient to have exactly the same transfer C objects for both C-APIs. IMO it doesn't make much sense to create new functions in the C-API just to maneuver around this. Instead we should indeed give some guidance which ranges are for what use. I already posted something in that direction:

>0 : headers that only define stuff
<=0 : headers that output css code
1 - 100 : for personal stuff
100 - 1000 : for framework stuff
1000 + : for distributed frameworks

We also could enfore certain ranges for plugins. But I don't know what you mean by "before the load path". IMO the use case where you simply have a list of importers/headers is possible, as libsass should execute same priorities in registration order (but I have to confess, I didn't really test this).

@chriseppstein is this a showstopper for you for the upcoming 3.2.0 release? The API is still marked highly experimental, so IMO it should be ok for now. IMO we can further discuss this to get to an agreement on a high and low value after we released a first "draft".

[xzyfer]

Apologies if I've missed something, I'm just catching up on all the recent changes.

I think we may have prematurely over-engineered the extensibility points. I think it's worth stepping back and questioning do we really need custom functions, importers, and headers? And what are the real world usecases they solve?

I'm struggling to see what the usecase for custom headers is? As @chriseppstein has mentioned custom mixins don't really make sense.

[xzyfer]

For the time being it's my opinion we should disable custom headers in 3.2.0.

Let's open an RFC to discuss the intention, usecases, and implementation of this feature. The C API is quickly becoming a major feature of Libsass and it's worth doing the due diligence on changes and features - this is especially important in cases where there is no Ruby Sass / Compass equivalent.

It's probably worth also opening an RFC regarding importer weights. I agree that it should be a list, rather than weighted. The pragmatist in me sees this quickly becoming a race to the top ala z-indexes.

[xzyfer]

Since this has been merged lets continue the custom import discussion to #1034