Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): add support for multiple extends on a simple selector by using composition #947

Closed
wants to merge 1 commit into from

Conversation

barak007
Copy link
Collaborator

@barak007 barak007 commented Jan 22, 2020

  • Add -st-compose to StylableDirectives
  • Process multiple extends into -st-compose after the first one
  • Reorder resolver object
  • Fix bug wrong meta for local classes from parent resolution
  • Export multiple composed classes

This feature will allow to write multiple -st-extends on parts. the first extends defines the type like before and the rest will be added to the JavaScript module exports.
Example:

.root {
  -st-extends: a b c;
}
.a {}
.b {}
.c {}

exported classes

{
  root: "root a b c",
  a: "a",
  b: "b",
  c: "c"
}

* Process multiple extends into -st-compose after the first one
* Reorder resolver object
* Fix bug wrong meta for local classes from parent resolution
* Export multiple composed classes
@barak007 barak007 requested a review from tomrav January 22, 2020 08:11
@tomrav tomrav changed the title Multiple extends Add support for multiple extends on a simple selector Jan 22, 2020
@tomrav
Copy link
Collaborator

tomrav commented Jan 22, 2020

A couple of questions about this:

  • -st-compose or -st-composes?
  • comma or space separated?
  • is there any difference in composing an inner part vs. a root? (e.g. named/default imports)
  • should we not expose -st-compose as its own directive as well? or is there no use-case for it without extending the type?

@tomrav tomrav changed the title Add support for multiple extends on a simple selector Add support for multiple extends on a simple selector by using composition Jan 22, 2020
@tomrav tomrav added core Processing and transforming logic feature New syntax feature or behavior labels Jan 22, 2020
@idoros
Copy link
Collaborator

idoros commented Jan 22, 2020

PR talks about -st-compose and also the code adds -st-compose, but the feature seems to be based on -st-extends.

@tomrav
Copy link
Collaborator

tomrav commented Jan 22, 2020

PR talks about -st-compose and also the code adds -st-compose, but the feature seems to be based on -st-extends.

Yes, his suggestion is it to implement -st-compose as a hidden directive and to use it through the multiple extends syntax.

@idoros
Copy link
Collaborator

idoros commented Jan 22, 2020

Didn't understand that from the description. So -st-compose is a real thing that can be used to compose classes without inheriting any type (when you say hidden, you mean undocumented)? and the new feature just appends the -st-extends 2nd...Nth classes to compose?

If true, then I'm not sure why you would add to the complexity of -st-extends. I would simply expose -st-compose as a way to allow class appends and not mix the features. We might want to expand extend in the future.

Also, what if a class that contains multiple composed classes is composed into another class, are all classes merged in?

@barak007
Copy link
Collaborator Author

-st-compose is an internal directive you cannot write it in css you should use -st-extends.
for now we are not implementing multiple type resolution (union/intersections) only the first extends is considered as type.
multiple composed classes are exported together.

.a{
  -st-extends: b;
}
.b{
  -st-extends: c;
}
.c{
  -st-extends: d e f; 
}

.d{}
.e{}
.f{}
.

export for a is: a b c d e f

@idoros
Copy link
Collaborator

idoros commented Jan 22, 2020

I understand that now. Still think it's a mistake to complicate -st-extends. Compose can be exposed without changing extend

@tomrav
Copy link
Collaborator

tomrav commented Jan 23, 2020

Let's say it's a separate directive, then the following example is valid?

.a {
  -st-extends: b;
  -st-composes: c;
}
.b {}
.c {
  -st-composes: d;
}
.d {}

export for a would be a b c d?

@idoros
Copy link
Collaborator

idoros commented Jan 23, 2020

export for a would be a b c d

Yes, I think this makes sense.

I will try to sum up my reasoning for exposing -st-compose and keeping -st-extends as is.

I think the current behavior of -st-extends is correct, but complicated as is. Using -st-extends on a class name causes 2 things:

  1. merge the type of the reference to the class
  2. append the reference output class name to export - only if its not a root of a stylesheet

The reason root classes are not appended is because we assume that the root of a stylesheet is a component, and since the component code doesn't assume anything about the code that uses it, it will append it's own style root. So if a root class is extended, no extra class name is passed, and there is no duplicate class name in the DOM. Another benefit is that Stylable warns in development when an extending class is not applied to the component it extends.

The current limitation with -st-extends is that it only accepts a single reference, that is because we didn't want to allow for union types. At Least for the moment.

I think bringing back -st-compose is good because it opens up more styling patterns that don't necessarily need the complexity we currently have with -st-extends (types, components pattern).

Keeping -st-extends as is will keep things simpler without making future changes much harder.

@tomrav tomrav added this to the Quality of Life - May 2020 milestone May 12, 2020
@tomrav tomrav marked this pull request as draft February 17, 2021 14:47
@tomrav tomrav changed the title Add support for multiple extends on a simple selector by using composition feat(core): add support for multiple extends on a simple selector by using composition Jul 13, 2021
@barak007
Copy link
Collaborator Author

close in favor of progress in #2299

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Processing and transforming logic feature New syntax feature or behavior
Projects
Status: ⏸️ Paused
Development

Successfully merging this pull request may close these issues.

None yet

3 participants