Import plugin

[rjgotten]

This pull requests implements @import (plugin) functionality as per #2416, which branched off from #1861.

Current implementation is NodeJS-only.
A small unit test is added that plugs in a custom function named "test", which simply outputs an Anonymous node with the literal "OK".

[matthew-dean]

Amazing! Is this only implemented as a root-level declaration? Is it possible to use this inside another file that is referenced with a regular @import?

[matthew-dean]

And, by the way, once an approach like this works for browser too, we could officially migrate people away from using inline JS / backticks in their Less files and put JS where it belongs. So that would be a nice-to-have, as that's a complete problem solved, but I think node-only is an okay first step. This is awesome work.

[rjgotten]

Amazing! Is this only implemented as a root-level declaration? Is it possible to use this inside another file that is referenced with a regular @import?

Due to how plugins currently work they are registered globally as soon as they are imported this way. So they should work from anywhere.

Imo that is actually a bit counter-intuitive (and slightly dangerous w/ regard to parallel import processing).

I'm thinking of limiting the scope of what type of plugins can be @import-ed to just functions, and sidestep the whole issue with globally registered pre-/post-processors; visitors; etc. Those don't lend themselves to localized import that well anyway. Usually you'd use them across an entire project in a wider fashion. E.g. property de-duplication or reordering, auto-prefix rewriting, minification, etc.

Functions are more easily localized: you simply avoid the global function registry and instead register @import-ed plugin functions on the import environment. Then you layer that set on top of the global less.functions.functionRegistry at run time to get the complete set of functions available at the place of a function call AST node.

I think cutting back on the API for @import-ed plugins to this level should also make it a good deal easier to get them registered in the browser. Basically, you'd give access to the Node subclasses from less.tree to compose output values and that would be it.

Leave it up to plugin authors if they need more (e.g. file access) and let them use a library like RequireJS to pull in those dependencies as asynchronous modules, if they must.

Basically; solve the easiest case with the biggest win first.

This is awesome work.

You're welcome. ;-)

[rjgotten]

@matthew-dean

I limited the functionality for @import (plugin) to defining custom functions.
(Probably should name it @import (functions) now, really...)

Indeed this made it more managable to get things rolling properly and as of now my pull request:

• has imported plugin functions working in the browser
• correctly pulls plugin functions that were imported inside a child sheet into a parent sheet
• allows importing plugin functions in nested ruleset scopes, and correctly limits them to those scopes

Proper tests should still be added for the following, but it should also:

• shadow an imported plugin function if it was already defined at a higher scope
• shadow globally registered functions, allowing people to monkey-patch the built-in set of functions

I think this is as far as I can take it for now.

[matthew-dean]

allows importing plugin functions in nested ruleset scopes, and correctly limits them to those scopes

😮

Holy crap. That is epicly amazing.

I think it would be fine to leave it as (plugin), and just describe the difference in documentation between a pre-loaded and an inline plugin. The reason I say that is that we don't want to get in a situation where more is added later to inline plugins to make them synchronous with pre-loaded plugins, and then we're stuck with calling them by two different names. Or, at the least, inline plugins might be extended to do something other than defining functions, and then the name (functions) is no longer accurate. Plugin is the more generic term.

But this:

Basically; solve the easiest case with the biggest win first.

Is exactly right. Solving this for a small case in a cross-platform way is most important, and this is a game-changing feature for Less authors.

@lukeapage is the best one to review pull requests, so I'd want him to take a look and see what he thinks about it. But I'd love to personally get my hands on this feature and see what I could do with it.

[jonschlinkert]

@import (functions)

awesome! my suggestion would to to call this @import (helpers) (or singular) which is exactly what you're describing.

[matthew-dean]

Traditionally, helpers provide additional functionality to existing methods. So an example of a helper might be a plugin that extends, say, the data-uri to accept an additional argument (like maybe only producing a data-uri based on some condition? who knows.) So yes, the example of monkey-patching existing less functions could be considered a helper. But the addition of the other use cases makes it a bit more generic than either "helpers" or "functions". For example, I could see plugins adding not just other functions alongside less functions, but perhaps additional syntax rules with custom syntax which generate custom nodes. It would really be up to the author (and what we allow).

[matthew-dean]

If we're worried about being semantically confusing between pre-loaded and inline plugins (which seems valid), what about a different "generic" like extensions?

@import (extension) "myFunctions.js";

Something like that? And then a JavaScript file which can either operate pre-loaded or inline could be a plugin AND an extension, or a JS file could be either one. That might make the distinction much clearer. What do you think?

AND, if we did it this way, then a plugin could actually make various extensions "available" to the .less files, so that people could use those functions in the scoped way that @rjgotten has created, as in:

// command line
lessc --plugin=myplugin.js

// .less file
.ruleset {
  @import (extension) "myplugin/colors";
}

Just another idea.

[seven-phases-max]

My only concern is that behind the scenes this patch really adds a new and never discussed feature of "scope-wise" built-in functions. And while it works like a charm (yes I did a few tests), the question is: do we really need that? Currently all built-in functions are always global so expecting a plugin to provide something else is not so expectable. I can see what benefits it could have in some distant future but for now it looks like just a few more of not that necessary code in the compiler making things just a bit more harder to maintain.
So (for now) I'd rather expect @import (plugin) "some-functions"; (if this plugin provides functions) to always attach to the global function registry (and probably throw a error if used not in global scope).

[matthew-dean]

The most obvious example is Less library authors. They may write plugins that add functions only relevant to the library, or plugins which may cause unexpected behavior for the end user if used globally. So, in that sense, plugins are really always local (in their intended use), not global. They're local to the author of the stylesheets. It's considered best practice in JavaScript and the de facto standard to limit global exposure of objects / vars / functions for the very reason of avoiding conflicts (and other reasons), so I'm not sure why that wouldn't apply here. I actually feel like Less currently doesn't give enough support for namespacing / avoiding global conflicts and assumes that one author is creating all style blocks, when that's rarely the case in large projects.

I would add that this was discussed, at least in the sense that it was an issue that came up of how to limit scope / reduce conflicts for plugins if used in libraries. No one had a solution, because I think it was assumed that plugins would have to be loaded globally, or at least I assumed so.

In practice, most people will, yes, probably declare a plugin globally. And probably conflicts would be rare anyway. BUT if I were writing a Less library (which I intend to do at some point when I magically have time), I would likely reduce most variables, mixins, and plugins to only apply locally to the library if I could, just so conflicts with someone else's code / plugin doesn't have to be an issue.

Local scope keeps Less code / files modular, and keeping inline plugins local keeps things consistent with other scoping rules. And, if someone wants a global plugin, you throw it in the global scope. Seems like a win all around.

[rjgotten]

but for now it looks like just a few more of not that necessary code in the compiler making things just a bit more harder to maintain.

There's about 5 to 10 lines of code difference between attaching everything to a single global registry or mainintaining a cascade of scoped function registries. The bulk of the changes is really in other areas: differentiating between a plugin import and other kinds of import, handling plugin JS file loading and execution cross-platform, ensuring the compiled functions stick around in the import cache, etc. all of which would still be necessary even when using a single global registry.

So (for now) I'd rather expect @import (plugin) "some-functions"; (if this plugin provides functions) to always attach to the global function registry (and probably throw a error if used not in global scope).

Throwing an error on a scoped import is the worst possible solution, imho, because it violates principle of least surprise: scoped import of LESS files may arbitrarily cause compilation to fail based on black-boxed details of the imported files. It breaks projects where 3rd party stylesheets pulled in via scoped import transitively import plugin functions themselves.

I would add that this was discussed, at least in the sense that it was an issue that came up of how to limit scope / reduce conflicts for plugins if used in libraries. No one had a solution, because I think it was assumed that plugins would have to be loaded globally, or at least I assumed so.

Yes. It was discussed in issue #1861. Back then I suggested that you could scope imported plugins to 'the scope of the importing sheet'. That was a bit crudely formulated, but what I meant there is exactly what my pull request is now doing; it's limiting the scope of the functions to where the importing sheet would've limited a normal import.

BUT if I were writing a Less library (which I intend to do at some point when I magically have time), I would likely reduce most variables, mixins, and plugins to only apply locally to the library if I could, just so conflicts with someone else's code / plugin doesn't have to be an issue.

This was my major motivator to get scoping working as well.

[rjgotten]

My bad; turns out there were still a few changes left behind from my earlier proof-of-concept implementations. I just updated the pull request to remove those and leave the files behind clean.

[jonschlinkert]

Traditionally, helpers provide additional functionality to existing methods.

http://blog.teamtreehouse.com/handlebars-js-part-2-partials-and-helpers. I was just throwing it out

[matthew-dean]

Yes, Handlebars is a good example of helpers because they add extra functionality to the {{ }} blocks, rather than adding additional templating features.

[jonschlinkert]

Yes, Handlebars is a good example of helpers because they add extra functionality to the {{ }} blocks, rather than adding additional templating features.

as you saying they're called "helpers" because of what they "can do", or because of where they are used?

[jonschlinkert]

to clarify though, I was saying that this pr adds a plugin that would pull in helpers. that's what it looks like to me. more to the point, the distinction of whether or not it's pulling in helpers or functions (same thing) or other plugins is a matter of conventions and what we'd like to see happen.

e.g when function A pulls in function B, what will B do? will it perform some kind of string transformation? I'm not sure it matters, b/c when a function is invoked in a string, is has the signature and behavior of a helper. I'm not saying that's technically correct, but it is potentially confusing if we decide to introduce other types of similar functionality later. Not everything needs to be on @import, it could be @plugin perhaps. just thinking out loud

[matthew-dean]

Not everything needs to be on @import, it could be @plugin perhaps.

Hmm, that's not a bad idea either. I guess currently import is used for styles, whereas this is something that would be applied TO styles. It would also shorten the syntax. Hmm.... Come to think of it, the parentheticals added to @imports were to add different options for HOW those styles were being imported. Might be too close to the same word, but could also do @include.

I think the first on the checklist is to address @seven-phases-max's concerns and to answer whether this is more or less structurally on target. I love it obviously, but @jonschlinkert do you weigh in on the feature overall. Maybe specifically the global vs. local question?

[jonschlinkert]

do you weigh in on the feature overall. Maybe specifically the global vs. local question?

I think it's great! I'm all for it, the pr and everything this represents.

[rjgotten]

Not everything needs to be on @import, it could be @plugin perhaps.

The major reason I implemented this feature with the @import (plugin) syntax, is that this resulted in the lowest impact on the existing parser.

Sure, it provides orthogonality with existing import syntax as well, but ofcourse; that's a double edged sword of sorts. As @matthew-dean already mentions: @import was meant for styles and using (abusing?) it as a language extension point may result in unwanted muddying of the waters.

Really; the more I think about it, the better a dedicated @include or @plugin sounds. (I'd avoid using @extension or @extend though, given the fact that :extend() already exists.)

[EDIT]

Aaaa--nd I just realized; if you want to resolve the global vs local dilemma; a dedicated keyword for plugin imports means you can give it a dedicated set of options. So you could introduce @include (global) and @include (local) and both have your cake and eat it. (For all aforementioned reasons, I still believe local scope should be the default though.)

This doesn't resolve @seven-phases-max's concerns regarding added compiler complexity, but as I've already mentioned; that's pretty much a moot point considering the difference between global scope or local scope is really, really minor implementation-wise.

However, it does resolve potential issues with confusion on the users' end.

[matthew-dean]

@rjgotten +1 to all of what you just said.

@seven-phases-max With the additional feedback, where are you at with this?

[seven-phases-max]

Well, my concern was not really in the "global vs. scope-wise" thing in particular but in "unnecessary code before it comes to play" (so far we have literally zero plugins in action to actually predict anything too far). Either way if everybody (cc: @LukePage) are OK with these changes I'm OK with that too.

---

Speaking of the syntax itself (i.e. @import (plugin) vs. @plugin or so) I suppose it does not really that important at all. I guess @import (plugin) is fine (even if for now it can import only functions) since the rest of functionality is easily grafted on top of it. Same for @plugin, though this one, while being a bit more more intrusive of course, at least provides a possibility to pass options to the plugin itself.

[rjgotten]

@matthew-dean:
Do you want to amend the pull request to support @plugin ?

Well, I can fold it in while still re-using the existing import manager class, I guess.
(I really don't think you'd want to duplicate the managing of file managers; the path lookups; the cache logic; etc. for plugins, would you?)

Then it's just a matter of splicing off the functionality I housed under the import node type into a dedicated plugin node type and writing parser logic for the new at-rule.

I've pretty much burned through company time I could spend on this feature, though, so it'd have to be something done in my off-hours during the evenings over the course of this week. If you're willing to wait on that, I can do it.

[jonschlinkert]

can we wait for @lukeapage to weigh in on this before making any further plans?

[matthew-dean]

The consensus is @plugin. So say we all.

can we wait for @lukeapage to weigh in on this before making any further plans?

Yep, he usually does final code review before merge either way. I don't think there's anything else to add at this point.

@rjgotten

I've pretty much burned through company time I could spend on this feature, though, so it'd have to be something done in my off-hours during the evenings over the course of this week. If you're willing to wait on that, I can do it.

With new syntax, it's important to get it right, so there's no hurry.

[lukeapage]

can we wait for @lukeapage to weigh in on this before making any further plans?

Sorry I had been busy with other things. The discussion makes sense to me, @plugin does sound more flexible.

re: local/global I think local support does put a burden of support on us, I think the simple case of only allowing it globally is best, however I remember detecting if you are in the global scope is harder than it sounds.

[rjgotten]

@lukeapage
I think local support does put a burden of support on us, I think the simple case of only allowing it globally is best

Forcing global puts burden on users and framework authors though: it leaves you with no way to isolate imported functions and with no way to prevent name collisions or resolve them according to a fixed rule system, i.e. , shadowing what is defined in a parent closure scope.

Consider the truly minute difference between local vs global scope: really; the only difference is in chaining in a new function registry when evaluating a ruleset node and appending to that registry via the context's top frame when the import/plugin node is evaluated, versus directly writing to the global registry. That boils down to what you do in two and only two lines of code, namely:

• https://github.com/less/less.js/pull/2479/files#diff-1c45ebc9468a9eb60b22aa690d2acb12R125
• https://github.com/less/less.js/pull/2479/files#diff-009c713136913e81ad994df5ac8b4961R47

There's one more place where the locally retained registry has some ugly logic attached and that's in the special case of mixin guards where a reference to it has to be created on the cloned ruleset:

• https://github.com/less/less.js/pull/2479/files#diff-833ffd28160e415fb9c7386ddcf87e6fR72

Arguably, the real problem there is not that reference, but that you lack well-defined, centralized clone methods on your parse tree's node classes to begin with. (I believe building these was mentioned in another recent issue.)

@seven-phases-max
(so far we have literally zero plugins in action to actually predict anything too far)

I'm planning to build list concatenation and dictionary (defined via list of list) key->value lookup functions, at the least.

[matthew-dean]

Thanks for chiming in @lukeapage. I still believe that in 99% of usage, it will be, in effect, global, because most people will put statements at the top of their sheets (and we can reinforce with examples and note scoping). And then for library authors, they can transparently scope plugins without having to do any special configuration. And, of course, with Less's ability to import something into scope, it's fairly trivial to make a plugin visible to global with a mixin.

I'd suggest we move forward with @plugin with local scoping, and see what happens. If there's a big demand for a global option, it seems like we could easily address it later.

[lukeapage]

What about visitor plugins? Pre/post process plugins? If we can get function plugins working local scope easily, maybe thats okay as long as we document other plugin types wont work in the same way (a warning?)

[rjgotten]

@lukeapage
My pull request explicitly only supports inline loading of additional functions. This was a conscious decision because the other types of plugin available currently only make sense on a global scale where they are passed in as switches to the compiler:

Pre-/post-processors only make sense on the level of the global input-/output-stream. Pre-processors in particular would be nasty to specify inline, as their very goal is altering the input stream which could be specifying them. You just end up in all kinds of unpredictible nastiness with recursion and the grandfather paradox. (An inline specified pre-processor that eliminates itself.)

While visitors can be handled a bit better in that they have strict different 'passes' over the initial AST in which they are applied, it's still possible for a visitor to mutate the AST in such a way that the same issues as with a pre-processor would arise.

Functions are really the only currently available plugin construct that's safe to load inline without affecting soundness of the compilation. But even so: it is still a very important change to support inline loading them.

Custom functions that can be loaded without needing explicit registration with the compiler via the API or via the commandline are a huge, huge leap forward in expressiveness for frameworks or libraries that try to handle and abstract away complex problems that cannot be viably covered by the core language.

[matthew-dean]

@lukeapage

If we can get function plugins working local scope easily, maybe thats okay as long as we
document other plugin types wont work in the same way (a warning?)

That's the approach that makes sense to me. Basically, just clarify in documentation that inline plugins can do A, and pre-loaded (command-line) plugins can do A and B. So instead of saying what inline plugins don't do, specify what pre-loaded plugins do "in addition to". It's a little bit easier to grok additional functionality than subtracted functionality.

[lukeapage]

Ok, sounds good. Guess we are just waiting for the change in syntax and me to code review?

[matthew-dean]

@lukeapage Yep, as far as I know. ^_^

[lukeapage]

code looks good so far

[rjgotten]

@lukeapage
I've amended the pull request with @plugin.

Under the hood it is still using an Import type tree node; there seems to be a lot of dependencies on the import visitor, sequencer and manager that I cannot resolve easily.

The AppVeyor build is currently failing, but it's a false negative: it's a failed NPM package installation for what seems to be the jscs package that's messing things up on the Node 0.10 build and a fatal error related to deleting a file that is in-use (Fatal error: EBUSY, unlink) on the Node 0.12 build, which is probably a race in AppVeyor's test harness set-up.

[lukeapage]

Any chance you could make a PR against the docs for this?

[lukeapage]

and thanks btw!

[matthew-dean]

Great stuff! Thanks @rjgotten! And @lukeapage! And everyone else who contributed, lol!

[rjgotten]

Any chance you could make a PR against the docs for this?

I can have a look tomorrow evening, I think.

[rjgotten]

@lukeapage
I just set up a second pull request #2522 to resolve a few remaining scope issues with @plugin and make the unit tests more complete to prevent future regressions.

I've had some time last week to write a small bit of the docs, but it's not complete yet. (Just letting you know it's still ongoing...)