Skip to content

Proposal for First Class Mixins: Draft 1#3442

Closed
K4rakara wants to merge 2 commits intosass:mainfrom
K4rakara:first-class-mixins
Closed

Proposal for First Class Mixins: Draft 1#3442
K4rakara wants to merge 2 commits intosass:mainfrom
K4rakara:first-class-mixins

Conversation

@K4rakara
Copy link

@K4rakara K4rakara commented Nov 9, 2022

Addresses issue #626.

There are still a few questions to address before merging this:

  • Should apply and get-mixin be global? If so, a deprecation section needs to be added.

  • What is a clear, concise example of why you might use this feature? Most of the applications I can think of are in very complex codebases, which are the exact opposite of concise 😅

  • Did I format the separation between functions and mixins correctly? I was initially going to put two toplevel sections, but decided to put them both under semantics, since thats what the contributing guidelines say.

@nex3
Copy link
Contributor

nex3 commented Nov 17, 2022

Addresses issue #626.

There are still a few questions to address before merging this:

  • Should apply and get-mixin be global? If so, a deprecation section needs to be added.

No, all new functions should be defined in built-in modules only unless they're intentionally polyfilling plain-CSS functions.

  • What is a clear, concise example of why you might use this feature? Most of the applications I can think of are in very complex codebases, which are the exact opposite of concise 😅

Don't worry too much about this; for the justification, you can just say that it meets a common user request and that it matches first-class functions.

  • Did I format the separation between functions and mixins correctly? I was initially going to put two toplevel sections, but decided to put them both under semantics, since thats what the contributing guidelines say.

I think making them top-level sections would be better, but don't stress out too much about the formatting either way.

@K4rakara K4rakara marked this pull request as ready for review November 29, 2022 21:07
@nex3 nex3 self-requested a review December 7, 2022 21:44
Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

Since this is defining an entirely new value type, it needs to have some additional machinery, including:

  • An explicit definition of what data a "mixin" value contains, under the Types section.
  • A definition of how the equality operation works for mixins.
  • A definition of how to serialize mixins, both as CSS (which should just throw an error) and for meta.inspect().
  • An update to meta.type-of() that returns "mixin" for first-class mixins.

You can see examples for all this in the first-class calc proposal under accepted/.


* `call()` → `apply()`

## Functions
Copy link
Contributor

Choose a reason for hiding this comment

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

The new functions and mixin should be added to the sass:meta module rather than the global scope. We're no longer adding new global functions except for CSS compatibility.

* Let `use` be the `@use` rule in [the current source file][] whose namespace is
equal to `$module`. If no such rule exists, throw an error.

* Return a map whose keys are the names of mixins in [`use`'s module][] and
Copy link
Contributor

Choose a reason for hiding this comment

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

The keys should be explicitly specified to be quoted strings.


* If `include` has a `ContentBlock`, it should be passed down to `$mixin`.

* Let `content` be the result of executing `$mixin` with `$args`.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear what it means to invoke a first-class mixin object with an argument list. Currently the only definition we have for what it means to execute a mixin comes from the @include Semantics section in mixin.md, which involves both resolving a name (which isn't necessary here) and executing an ArgumentInvocation (which we don't have).

To handle this, we should probably just the relevant part from the "Execute a mixin" section and repeat it here, which it looks like is just:

  • Assign $args to $mixin's ArgumentDeclaration in $mixin's scope.

  • Execute each statement in $mixin.

(At some point we should flesh out what that first bullet point means, but we already rely on behavior there for meta.call() so it's fine to leave it out of this proposal.)

Another benefit of this is that you can actually leave out any explicit specification of the ContentBlock, since the @include rule that was used to invoke $mixin is actually meta.apply()'s include rule! It's probably a good idea to point that out in a block quote, though.

@nex3
Copy link
Contributor

nex3 commented Dec 15, 2022

Ideally, I'd also like to have a JS API type added for this as well. We've been falling behind on ensuring the JS API supports all Sass value types, so I'd like to avoid furthering that problem here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants