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

Question: why does external compose all the classes into a selector? #727

Closed
jquense opened this issue Feb 21, 2020 · 7 comments
Closed

Comments

@jquense
Copy link
Contributor

jquense commented Feb 21, 2020

Just a general question about the original rationale for having :external compose all the clases the references composes and not just the class matching the identifier:

.wooga { color: red; }

.booga {
  composes: wooga;
}

// other file
.b :external(booga from  "./simple.css") {
    background: blue;
}

Produces:

.b .wooga.booga {

instead of

.b .booga {

The current approach it seems maybe more "correct" somehow, but also leads to selectors whose specificity is hard to reason about (and can change indirectly). Wondering if there were specific practical considerations or if you've seen benefits vs downsides to this approach?

@tivac
Copy link
Owner

tivac commented Feb 21, 2020

I haven't run into any downsides of the current approach, your suspicion is correct in that including all of the dependencies feels more correct and unsurprising to me. They could change out from underneath you, yes, but that's why :external() should be used carefully and sparingly.

Is the current behavior causing issues for you?

@jquense
Copy link
Contributor Author

jquense commented Feb 24, 2020

It's not causing issues in the sense something is broken, but it is leading to confusing css, and more bytes. Specifically we use something like tailwindcss where you can have a lot of classes composed in:

.foo {
  composes: pt-5 pb-6 mx-6 bg-red leading-wide from global;
}

you end up with a long selector that isn't necessary. I also see the other side of this which is that composed classes aren't really part of the thing you are trying to reference, they are an implementation detail, and having to think about them feels a bit off. In any case it's not actively a problem just something we noticed! Will report back if i find something technically limiting

@tivac
Copy link
Owner

tivac commented Feb 24, 2020

Certainly seems like you could run some experiments with changing that by modifying this to only take the... final element from file.exports[name] instead? Seems like that'd do what you're asking for and provide valuable data on whether or not this a) works and b) is a good idea. it still seems like a breaking change but I'm not fundamentally opposed to it if there's good arguments for making the change.

file.exports[name].forEach((value) =>
s.append(selector.className({ value }))
);

@jquense
Copy link
Contributor Author

jquense commented Feb 24, 2020

yup will do some research, wanted to make sure i had all the context first 👍

@tivac
Copy link
Owner

tivac commented Mar 31, 2020

@jquense Any updates on this?

@jquense
Copy link
Contributor Author

jquense commented Mar 31, 2020

no real updates. We've tried both approaches and they seem to work equally well with one producing less css so that's nice. Data is super anecdotal tho, I don't know if it'd break other people or lead to some intractable case. I would consider it tho, smaller, less complex output is a nice win, not sure tho if there is a hidden cost i'm not seeing

@tivac
Copy link
Owner

tivac commented Apr 8, 2020

I'm open to a PR changing this behavior :)

@tivac tivac closed this as completed Feb 3, 2022
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

No branches or pull requests

2 participants