-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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: allow :global
in more places
#12509
Conversation
…. }`, so the latter should be allowed, too
…ge and was since reverted), add new breaking change note
🦋 Changeset detectedLatest commit: d11556c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Irrelevant comment: @dummdidumm , I enjoyed your explanation even if I don't understand all of it (back-end dev here). It feels good to read this because it gives me confidence on Svelte, which is the technology I made my company bet on. Thank you very much. Keep it up. |
packages/svelte/src/compiler/phases/2-analyze/css/css-analyze.js
Outdated
Show resolved
Hide resolved
sites/svelte-5-preview/src/routes/docs/content/03-appendix/02-breaking-changes.md
Outdated
Show resolved
Hide resolved
Unfortunately I don't think this is quite right just yet. Consider a case like this: <main class="a">
<div class="b">red</div>
</main>
<main class="c">
<div class="d">blue</div>
</main>
<style>
/* this... */
div :global {
&:is(.a .b) {
color: red
}
}
/* desugars to this, but they behave differently */
div :global:is(.c .d) {
color: blue;
}
</style> This is what gets generated, on this branch: div.svelte-88fhbx {
&:is(.a .b) {
color: red
}
}
div.svelte-88fhbx :is(.c .d) {
color: blue;
} It seems what we really want is this: -div.svelte-88fhbx :is(.c .d) {
+div.svelte-88fhbx:is(.c .d) { IIUC we could achieve this by simply removing the descendant selector before any With that change, I think we could continue to disallow |
Good points - I'll try to tweak it |
This is a really nice PR, but I would feel better if it had some more test coverage around it so that we can be confident about not regressing things in the future. :) |
…breaking-changes.md Co-authored-by: Rich Harris <rich.harris@vercel.com>
I first thought that we can, but
This works out nicely except for one case: /* this */
div {
:global.x { ... }
}
/* desugars to this */
div :global.x { ... }
/* which means this */
div.svelte-hash.x { ... } Since this would require a very elaborate code mod in the CSS output to make these the same, and because it certainly doesn't look like it would desugar to this for users, we need to make this a compiler error. |
The more I think about it the more I think the idea of removing the spaces before div :global.x { ... }
/* ... looks like div .x, but with the "remove spaces before :global" idea it would actually be div.x */
div {
:global.x { ... }
}
/* ... desugars to the first example, but looks even less like the "remove spaces" idea */ Instead we need to disallow certain parent selectors ending with div :global {
p { ... }
}
/* desugars to */
div :global p { ... }
/* which is */
div p { ... }
/* therefore ok */
div :global.x {
&.y { ... }
}
/* desugars to */
div :global.x.y { ... }
/* which is */
div .x.y { ... }
/* therefore ok */
div :global {
&.x { ... }
}
/* desugars to */
div :global.x { ... }
/* which is */
div .x { ...}
/* which is different to what we would output in the nested case, therefore error */
/* instead you would need to do */
div:global {
&.x { ... }
}
/* which desugars to */
div:global.x { ... }
/* which is */
div.x { ... }
/* therefore ok */ In other words, the rule is: If you have a selector ending with a lone The alternative would be to turn div :global {
p { ... }
}
/* is like */
div * {
p { ... }
}
/* desugars to */
div :global p { ... }
/* which is */
div * p { ... }
/* therefore ok */
div :global.x {
&.y { ... }
}
/* desugars to */
div :global.x.y { ... }
/* which is */
div .x.y { ... }
/* therefore ok */
div :global {
&.x { ... }
}
/* is like */
div * {
&.x { ... }
}
/* desugars to */
div *.x { ... }
/* which is */
div .x { ...}
/* therefore ok */ This might actually be the most in line with how CSS works, but of course it means you're likely gonna do |
I'm not following — those aren't equivalent?
I'm not sure I see this as a problem? Assuming The compiler error feels problematic given the original issue — if someone wrote this... main :global {
@apply blah;
} ...the resulting error would be rather confusing.
We can't do that — I really would love for us to avoid |
They are, because the whole
It is a problem not because of preprocessors but because of what the final CSS looks like - you write the same in different variants (one nested, one not) which are semantically the same but you get different results. But it occured to me that we could prepend a Still, I find it deeply confusing that |
I don't understand at all. Why does that mean that
I'm not following this either, can you give an example?
Which output?
Yes, on the basis that no-one should be writing that CSS — it's preprocessor output only. In an ideal world (in which preprocessors weren't unnecessarily de-nesting stuff) we wouldn't allow it at all. |
div { :global { &.x { ... } } }
/* Svelte turns this into */
div { &.x { ... }
/* which is equivalent to */
div.x { ... }
/* therefore we need to allow */
div:global.x { ... }
/* which also turns into div.x */ (hashes omitted for brevity) But you're right that if we would turn
What I meant is that /* this */
div { :global.x { ... } }
/* is turned by Svelte into this */
div { .x { ... } }
/* which is the same as this */
div .x { ... }
/* but given the "remove descendant selector rule this */
div :global.x { ... }
/* would turn into this */
div.x { ... }
/* but */
div :global.x { ... }
/* is equivalent to */
div { :global.x { ... }
/* which as we saw above Svelte turns into something different -> they are not equivalent */ (hashes omitted for brevity)
Following up to the previous example /* this */
div { :global.x { ... } }
/* could be turned by Svelte into this */
div { &.x { ... } }
/* which is then the same as */
div.x { ... }
/* which could also be the result of */
div :global.x { ... }
/* so they would be equivalent */
For me it's less about preprocessors and more about semantical correctness. If I know the rules of descendant selectors (the pretty basic ones, about spaces meaning "child at any depth") and then |
I created #12560 again implementing the "remove preceeding descendant combinator" so we can compare side by side and then make a decision. |
merged #12560, closing |
Today, a selector like
div { :global { &.x { .. } } }
is valid, but the equivalent selectordiv:global.x { ... }
throws a compiler error. Apart from being inconsistent, this poses a real problem because some CSS preprocessors will apply (rightfully) CSS de-nesting, which meansdiv { :global { &.x { .. } } }
is turned intodiv:global.x { ... }
.With this PR, the placement of
:global
(but not:global(...)
) is relaxed and it can now appear anywhere inside a selector, just like regular pseudo classes can.Fixing this also fixes #10513 because Tailwind users are now unblocked when using
@apply
with variants, likedark:
. They can now write this...which tailwind's preprocessor will turn into something like this
(note how
:global
is betweenmain
and:is
, which is valid CSS but wasn't valid Svelte up until now)(doing
main :global { .. }
wouldn't work because it would be turned intomain :global:is(...) { .. }
which wouldn't match any elements)...which Svelte can then turn into this:
... which satisfies all constraints and works correctly/as expected.
Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests and linting
pnpm test
and lint the project withpnpm lint