detached rulesets

[lukeapage]

detached rulesets as discussed on a few issues.

See tests.

Knocked up in an hour, needs some thoughts on if this is the best syntax and whether we go ahead and some extra tests and anything else I may have missed.

[seven-phases-max]

Wow! That was fast. Starting to experiment with it right now.

[lukeapage]

One bug I forgot to put a todo on is you can't assign the ruleset to a
specific argument. 1 line fix though. If you do any tests as part of your
playing feel free to add a commit to this branch even if the test fails -
its on a branch on the main repo.

[lukeapage]

Okay I'd like to merge this. Its a big addition so is everyone happy @jonschlinkert @SomMeri @matthew-dean

[seven-phases-max]

So far I found two major issues:

#1 Parens inside {} block cause parser to fail:

.mixin(@v) {}

.mixin({
    width: (100px); // ParseError: Unrecognised input
});

#2 New syntax also (seems to) introduces {} and () combinations not supported by the chunk splitter so when code grows bigger this also may lead to mysterious Unrecognised input errors here and there (I don't have a good example yet).

[matthew-dean]

This is cool, but I still somewhat prefer passing mixin references, as it doesn't introduce a new type of syntax, and it also allows you to interpret and pass variables within rulesets. This introduces a new type of paradigm that could potentially be confusing if mixin references are added, as it looks like a mixin call in order to output it, which is different usage than typical variable output. However, the @ruleset() call WOULD make sense if @ruleset was a mixin reference.

I'm guessing you took this approach because it's easier to implement, but I still feel like @seven-phases-max's syntax (with your additions) is more logical, as it doesn't introduce a new "idea" like this does. It's just assigning / passing a mixin to a variable, and then calling it like a regular mixin.

So, for clarity, this makes sense to me:

.ruleset() {
     color: black;
     background: white;
 };

.wrap-mixin(@ruleset) {
  .wrap-selector {
      @ruleset();
    }
};

.wrap-mixin({
  color: black;
});

.wrap-mixin(.ruleset);

EDIT: However, to be fair, passing in a ruleset to a variable or assigning it directly to @ruleset is probably a distinction without a difference. I guess what I'm missing is the ability to do normal mixin stuff on these rulesets like guards and variables, but maybe this is a fine first step. Just my thoughts.

[seven-phases-max]

Unfortunalty I'm a bit limited in time for the moment to craft a prototype for that "by reference" feature (Hoping to make it in a few weeks or so). Either way I don't think we have to either choose between them or to not release one before the other. So far I don't see any syntax conflicts in between so the other one can be transparently added later if everything goes OK. (If i'm not mistaken the only big disputed thing in the end was the necessity of a selector funtion i.e. .wrap-mixin(.ruleset); vs. .wrap-mixin(selector(.ruleset));)

[lukeapage]

Passing mixin references as I said before does introduce new syntax and in
fact imo is more risky for the parser going forward. Plus that means anyone
using this has to make 2 mixins.. the primary usecase is defining a wrap
mixin and having to pass a mixin in practically defeats the point of the
feature by forcing the user to write bloat.

[lukeapage]

Matthew, regarding primary usecases see #1867 - and there are lots of other
examples in our issues, but I haven't seen any examples of solid usecases
for passing mixins - though I do think its cool and we should consider that
as an extension of this.

[SomMeri]

@lukeapage I do not have much time now, so I looked only on detached-rulesets.less test and I liked it. Is there going to be beta release with new features or something like that? I can try to break it next week or so.

[lukeapage]

@seven-phases-max I changed that < 512 on the chunker to < 1 and then ran it again. I'll get that fixed and then I'll move onto the brackets.

I suspect most of the problems are we don't have multiple fallbacks for chunker - there are a whole load of hacks in the parser to handle not being able to memo and restore multiple times, so if need be I might fix all of that.

[lukeapage]

@SomMeri I could release a beta version but generally I find people don't seem to use it. Instead I'm trying the opposite - releasing more often and fixing bugs in minor releases, so that people who want stability are best off going with the previous major release.last minor release. But I guess that could be better described on our website!

[matthew-dean]

@lukeapage Makes sense, I was thinking about this more this morning. This still does provide a lot of powerful and useful scenarios, and we could close off a LOT of stuff involving magic @content vars and passing rules into @media blocks within mixins (which should be part of added test cases), and maybe even some stuff around keyframes (although I haven't worked through it). And it's much better than a multi-line string to solve the problem, so it's still pretty awesome. Nice work.

[nelsonpecora]

+1 This is fantastic, and coincidentally is exactly what I need to implement a few things for a project I'm working on (making a better .hover() / .active() / .focus() mixin that will cut down on crufty code when we want those states to be responsive).

[lukeapage]

Okay, I think thats alot more solid - should have fixed. Last thing on my todo list if no-one finds another issue with this branch is to add some test cases for error scenarios - using the detached ruleset as a property value etc!

[seven-phases-max]

Just in case, here I published a few test/use cases for this feature and #1860 (and most likely #1957 is going to climb there aboard too).

[lukeapage]

cool, take it there weren't any problems? I expanded the tests a bit more and wrote tests for errors. I'll call this ready to go and merge it in in a couple of days

[seven-phases-max]

Everything I'd expected to work works like a charm. The only uncertain visibility thing I stepped into was:

// @d: 2px;

.wrap-mixin(@ruleset) {
    .wrap-selector {
        @d: invisible;
        @ruleset();
    }
};

.wrap-mixin({
    two: @d; // undefined
});

At first I expected it to inherit @d variable from the .wrap-mixin scope but now I'm not sure. Assuming that the block works like plain CSS ruleset (and not as a parametric mixin), i.e. it is evaluated at the point of its definition, @d is probably expected to be undefined there.

On the other hand the following example still puts that in doubt:

@d: 2px;

.wrap-mixin(@ruleset) {
    .wrap-selector {
        @d: invisible;
        @ruleset();
        .plain-selector;
    }
};

.wrap-mixin({
    two: @d;
});

.plain-selector {
    three: @d;
}

result:

.wrap-selector {
  two: 2px;
  three: invisible;
}
.plain-selector {
  three: 2px;
}

(I.e. .plain-selector inherits caller's scope when used as mixin).

[lukeapage]

Yes, I added a test case for exactly this, and its as I would expect..
otherwise a mixin implementation could change the result of rulesets passed
to it. We could reverse the scope so it only uses the local scope if its
not in the global but I think I'd prefer your other suggested approach of
passing parameters to a passed mixin.
Its kind of the sane imo as calling a mixin and local scope where you call
not being available to the defined mixin ( I assume.. maybe need to
check!....)

[SomMeri]

@seven-phases-max Mixins normally search for variables/mixins at the point of their definition and when that fails, mixin will search in caller scope. It is as if mixins would have many "hidden" parameters. I think that it was intentional and not a bug, although I can not find issue where this was discussed.

Rewriting your example:

.wrap-mixin() {
    .wrap-selector {
        @d: invisible;
        .mixin(); //mixin is not available here
    }
};


.otherscope {
  .mixin() {
    two: @d;
  }
  .wrap-mixin(); //search will continue in this scope and mixin will be found
}

compiles into:

.otherscope .wrap-selector {
  two: invisible;
}

So, I think that the above behavior is consistent with how mixins behave.

[lukeapage]

@SomMeri I read your example as supporting @seven-phases-max and suggesting maybe we should fallback to allow searching in the current scope - because thats what mixins do.

tbh I didn't realise that example worked.

Should we keep it simple for now and consider adding support later (since we can always add but not take away) ?

[SomMeri]

@lukeapage That depends on how much work it requires I think. This is useful feature many people want, so it should not be hold back without strong reason.

In the long run, they should work the same way. It is easier for both users and coders that way. It is better if people do not have to remember all kinds of special cases and situations. I would be also perfectly ok with making mixins not to see callers scope - but that is necessary for now and would break many existing less projects, so it is not an option.

Getting less scoping and variables right is already complicated enough, especially after you make things like #996 to work or after you make interpolated imports see also variables defined after them. It is the thing I spend most time on, anything else is comparatively straightforward. There always seem to be one more unexpected special case, so I'm trying to minimize them.

It will get even more complicated with #1848.

[seven-phases-max]

Should we keep it simple for now and consider adding support later.

I guess it's better to add then if everything else is actually inheriting the callers scope. Unless this becomes too complicated already to bother (that's quite minor issue so we can always say "experimental", "partially implemented" and "not fully stabilized" yet :)

[lukeapage]

For this feature it adds quite a bit of complexity - we have to wrap the ruleset in something that refuses to evaluate it properly (but stores scope) until its called and then adds the calling scope to the bottom of the stack! so I would leave it for now but consider it a known issue (then if its easier for less4js to be consistent it can be)

[lukeapage]

For this feature it adds quite a bit of complexity - we have to wrap the ruleset in something that refuses to evaluate it properly (but stores scope) until its called and then adds the calling scope to the bottom of the stack!

I am in the process of doing this - it seems it is required for some media query combinations. I am going to change mixin definition to work in exactly the same way (instead of being special cased inside the eval of ruleset)