Feature: Implement multiple Custom Importers support

[am11]

Corresponds to sass/libsass#1000.

[am11]

Removed Breaking Change tag, because I will try to implement it in a backward compatible way (although libsass API marked it as a breaking change).

//cc @jhnns

[am11]

On second thought, instead of providing multiple variants:

(Note for priority precedence set by libsass: highest first)

// variant 1
importer: function(url, prev, done) {} // backward compatible 
// variant 2
importer: { priority: 1, func: function(url, prev, done) {} }
// variant 3
importer: [
  {
    priority: 1, func: function(url, prev, done) {}
  },
  function(url, prev, done) {},
  {
    priority: 100, func: function(url, prev, done) {}
  }
]
// etc.

How about aligning importers (plural) with custom function (and retiring importer singular):

importers: {
  10: function(url, prev, done) {},
  9: function(url, prev, done) {},
  2: function(url, prev, done) {}
},
headers: { // custom headers, similar to custom importers, upcoming feature: #822
  4: function(/* not sure yet */) {},
  1: function(/* not sure yet */) {}
},
functions: {
  foo1: function() {},
  'bar1($a)': function(a) {},
  'foo2($d, $c)': function(a, b) {},
  'bar2($apa = \'alpha\', $bao = \'bravo\')': function(x, y) {}
}

with proper validation messages. This will be another breaking change of v3. :)

Thoughts?

//cc @xzyfer

[xzyfer]

I prefer the latter on right way approach.

I presume the keys map to priority, identifier, and signature respectively? i.e. headers object keys should be strings not numbers.

[xzyfer]

/cc @Snugug

[am11]

Agree for importers.

I haven't looked into headers yet. @mgreter said it has similarities to importers, so I thought it would have priority attribute. But seems like it is more inclined towards functions. Lets discuss headers separately over #822. :)

[jhnns]

Thx for letting me know.

I'm a little concerned that things might get too difficult on your side because there are so many possibilities to use your API. But as long as you can handle it... 😀

[Snugug]

I agree I like the latter right way with importers. The only thing I don't like is priority; this conversation has been happening in Eyeglass (ping @chriseppstein and @jakobo) and we all kinda agree we really hate weighting as a sort method. Coming from the Drupal world, I can pretty much say it rarely works out as expected or wanted. I'd much prefer a simple array to explicit weights.

[mgreter]

@am11: from the implementer side there is not difference between custom headers and importers.
They only behave different (only one importer can return imports, while all headers can add imports).

As for the API, I also have some suggestions, as I really don't like the API that was proposed earlier:

importers: {
  10: function(url, prev, done) {},
  9: function(url, prev, done) {},
  2: function(url, prev, done) {}
}

While it may look nice at a first look, it brings one problem to the API. And in the end it makes it more complex than needed. What I mean is that you cannot easily extend such an object, since you could step on someone elses toe when using the same prio. Also if you have a really big list, it can get difficult to see which priority-key is not yet used. IMHO it's an abuse of the hash map 😄

I personally would rather expect something like this:

function importer1(..) {
  ...
}
importer.priority = 9;
...
headers: [
  importer1, // prio from function or default
  [importer1, 2], // prio from array or function or default
  { fn: importer1, prio: 3 } // prio from key or function or default
]

This allows to attach the priority to the function object itself, so you can pass it around, make copies, etc. This also makes it very flexible and should allow simple and complex scenarios. I would also like to allow headers to only contain a function (I do this all the time in JS for options that can be arrays).

@am11 not sure if you already do it that way, but I always just normalize input arguments into the expected structure. This should keep the code more readable as it seperates the real execution from the configuration options. For the options above this could look like this:

function importer0() {}
function importer1() {}
importer1.prio = 9;

function normalize_import_list(importers) {
  // wrap single item in array
  if (!Array.isArray(importers)) {
    importers = [ importers ]
  }
  // it has to be an array by now
  var i = importers.length; while(i--) {
    // skip items that already are arrays
    if (Array.isArray(importers[i])) continue;
    // object passed as importer
    if (typeof importers[i] === "object") {
      // object has a specific prio set
      if (typeof importers[i].prio !== "undefined") {
        importers[i] = [ importers[i].fn, importers[i].prio ]
      }
      // just add the function
      else {
        importers[i] = [ importers[i].fn ]
      }
    }
    else {
      importers[i] = [ importers[i] ]
    }
  }
  // second run to assert valid prio
  var i = importers.length; while(i--) {
    // maybe assert header function type
    // skip items that already has a valid prio
    if (typeof importers[i][1] !== "undefined") continue;
    // function object has a prio set
    if (typeof importers[i][0].prio !== "undefined") {
      importers[i][1] = importers[i][0].prio;
    }
    // set default
    else {
      importers[i][1] = 0;
    }
  }
  return importers;
}

console.debug(normalize_import_list(importer0))
// [[importer0(), 0]]
console.debug(normalize_import_list([importer0]))
// [[importer0(), 0]]
console.debug(normalize_import_list([[importer0, 42]]))
// [[importer0(), 42]]
console.debug(normalize_import_list({ fn: importer0 }))
// [[importer0(), 0]]
console.debug(normalize_import_list({ fn: importer0, prio:33 }))
// [[importer0(), 33]]
console.debug(normalize_import_list([{ fn: importer0, prio:33 }]))
// [[importer0(), 33]]
console.debug(normalize_import_list(importer1))
// [[importer1(), 9]]
console.debug(normalize_import_list([importer1]))
// [[importer1(), 9]]
console.debug(normalize_import_list([[importer1, 42]]))
// [[importer1(), 42]]
console.debug(normalize_import_list({ fn: importer1 }))
// [[importer1(), 9]]
console.debug(normalize_import_list({ fn: importer1, prio:33 }))
// [[importer1(), 33]]
console.debug(normalize_import_list([{ fn: importer1, prio: 33 }]))
// [[importer1(), 33]]

Once you got that sorted it should be straight forward to implement the rest!

[am11]

Guys please consider the real life use-case: average and the highly irregular one and then retake a look at what this "multi-importer" feature means from implementer perspective (which IMO is an "utterly auxiliary" feature). It is easy to get lost in varieties of syntax and drift away from the main subject.

@jhnns, ICYMI, we already have that going on in node-sass@beta with custom functions feature. 😸

@Snugug, IMO, node-sass should not filter out features whatever libsass API has to offer. So if libsass modifies and discontinues it, node-sass will/should too. The idea really is to keep node-sass act like a thin JS wrapper around libsass and give room to downstreams to maneuver around and introduce the "fancy stuff" the way they feel best.

@mgreter, thanks for the suggestion. I have following concerns with auto-normalizing and this approach sass/libsass#1000 (comment):

• from our perspective: there are too many combinations to manage for us and yet issues and more "versatile" (and IMO unnecessary) feature-requests in future.
• learning curve for implementer (which can be avoided here and avoiding is better than imposing it IMVHO).
• loss of flexibility: if libsass API changed again, node-sass being too tied to libsass, it will change as well. This is the lifestyle that we are aware of. But if node-sass invent something new as in giving variety of usage on top of libsass features and then the feature change in libsass, the affect in that case is exponential.

IMO we can keep things simple by not worrying about the apparent looks of syntax, but focus on simplicity of usage while bringing harmony in syntax with the closely related features (custom functions, custom headers, custom importers).

My revised version:

// custom function
functions: {
  test: function(x, y, z, a, b, c, blah, foo, bar) { /* body */ },  // I specifically
  // asked for this variant because IMO this is the most natural JS way
  'test1($a)': function(a) { /* body */ } // and so forth
}
// custom importer
importers: { // its the same
 10: function(cur, prev, done) { /* body */ },
 9: function(cur, prev, done) { /* body */ },
 8: function(cur, prev, done) { /* body */ },
 8: function(cur, prev, done) { /* body */ }, // duplicate! a JS user know
  // what happens when the dup key happens: the second take
  // precedence and the first one is discarded
 2: function(cur, prev, done) { /* body */ }
}

it can get difficult to see which priority-key is not yet used

Same reason can be applied to custom functions, options and any usage of object literal in JS. So its users' fault if they are failed to manage priorities for their use-case where they need "extra-ordinary" number of importers. It is like any object literal with massive number of members.

PS: Personally I might never use more than one importer with node-sass in real application, but handle all flavors of "url type" with one simple state-machine function (or call other functions within one driver function to keep the function size and cyclomatic complexity contained).

[Snugug]

@am11 My point was that weighting shouldn't be something that a user needs consider; it's a PITA DX to manage by hand, especially when an array works equally well.

[am11]

@Snugug, and my point is libsass should provide this auto-weighing and node-sass should not invent it on top of this existing libsass' experimental feature...

[xzyfer]

I'm 👍 for custom importers (multiple) and custom functions.

However considering the current discussion around custom headers (sass/libsass#1000 (comment)) I'd want to suggest keeping them out of node-sass@3.0.0 stable.

I agree with @chriseppstein in that this feature may not have the intended utility.

Guys please consider the real life use-case

I'm not convinced this has been done either here on in the Libsass project. I'm not currently convinced custom headers make sense as a feature.

[am11]

I'm not currently convinced custom headers make sense as a feature.

I was referring to the implementation details multiple custom importers' priority in light of comments above.

[am11]

Addressed by 47d6386 via #832.