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: allow :global in more places #12509

Closed
wants to merge 12 commits into from
Closed

feat: allow :global in more places #12509

wants to merge 12 commits into from

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Jul 19, 2024

Today, a selector like div { :global { &.x { .. } } } is valid, but the equivalent selector div: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 means div { :global { &.x { .. } } } is turned into div: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, like dark:. They can now write this

- main {
+ main:global {
	@apply bg-blue-100 dark:bg-red-100
}

...which tailwind's preprocessor will turn into something like this

main {
  background: blue;
}
main:global:is(dark-mode *) {
  background: red;
}

(note how :global is between main 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 into main :global:is(...) { .. } which wouldn't match any elements)
...which Svelte can then turn into this:

main.svelte-hash123 {
  background: blue;
}
main.svelte-hash123:is(dark-mode *) {
  background: red;
}

... which satisfies all constraints and works correctly/as expected.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Jul 19, 2024

🦋 Changeset detected

Latest commit: d11556c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

@webJose
Copy link

webJose commented Jul 19, 2024

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.

@Rich-Harris
Copy link
Member

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 :global when we transform the output. That way, we wouldn't need to ask people to do main:global — main :global would work (and is logical, since we're saying that @apply should be 'evaluated' in the global scope, aka the Tailwind scope, rather than the component scope).

With that change, I think we could continue to disallow selector:global, which is a bit confusing (since it looks like it's modifying selector but is actually modifying everything after it).

@dummdidumm
Copy link
Member Author

Good points - I'll try to tweak it

@trueadm
Copy link
Contributor

trueadm commented Jul 20, 2024

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>
@dummdidumm
Copy link
Member Author

With that change, I think we could continue to disallow selector:global

I first thought that we can, but div { :global { &.x { ...} } } is allowed, so div:global.x must be, too.

IIUC we could achieve this by simply removing the descendant selector before any :global when we transform the output

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.

@dummdidumm
Copy link
Member Author

dummdidumm commented Jul 22, 2024

The more I think about it the more I think the idea of removing the spaces before :global is confusing:

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 :global (omitting the class hashes for brevity):

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 :global, and the nested rules contain selectors starting with &, then it's a compiler error, and you instead need to append the :global to the end of the previous selector.

The alternative would be to turn :global into * when it's on its own:

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 div:global { ... } a lot more compared to div :global { ... }. So the common case is likely better served with going the compiler error route. (and we would also need to make an exception for lone top level :global entries; those shouldn't create a *)

@Rich-Harris
Copy link
Member

I first thought that we can, but div { :global { &.x { ...} } } is allowed, so div:global.x must be, too.

I'm not following — those aren't equivalent?

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 { ... }

I'm not sure I see this as a problem? Assuming :global.x is something that would only ever be written by a) a preprocessor or b) someone who was ignoring the documentation, that seems ok.

The compiler error feels problematic given the original issue — if someone wrote this...

main :global {
  @apply blah;
}

...the resulting error would be rather confusing.

The alternative would be to turn :global into * when it's on its own:

We can't do that — div :global p means 'any p inside a local div', not 'any p inside any element inside a local div'.

I really would love for us to avoid selector:global — it's so deeply at odds with how pseudo-class selectors like :active normally work (i.e. modifying the current selector) that no-one should ever have to write it.

@dummdidumm
Copy link
Member Author

I'm not following — those aren't equivalent?

They are, because the whole :global selector is removed, so the final CSS is div { &.x {...} } which is div.x, so therefore div:global.x must be allowed.

I'm not sure I see this as a problem?

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 & to the output to make them equivalent again.

Still, I find it deeply confusing that div :global.x would actually mean div.x - more than div:global meaning something different compared to things like :active. Am I understanding you correctly that you rather have the div :global.x -> div.x behavior?

@Rich-Harris
Copy link
Member

They are, because the whole :global selector is removed, so the final CSS is div { &.x {...} } which is div.x, so therefore div:global.x must be allowed.

I don't understand at all. Why does that mean that div:global.x must be allowed?

you write the same in different variants (one nested, one not) which are semantically the same but you get different results

I'm not following this either, can you give an example?

But it occured to me that we could prepend a & to the output to make them equivalent again.

Which output?

Am I understanding you correctly that you rather have the div :global.x -> div.x behavior?

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.

@dummdidumm
Copy link
Member Author

dummdidumm commented Jul 23, 2024

They are, because the whole :global selector is removed, so the final CSS is div { &.x {...} } which is div.x, so therefore div:global.x must be allowed.

I don't understand at all. Why does that mean that div:global.x must be allowed?

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 div :global.x into div.x then we don't need to allow it.

you write the same in different variants (one nested, one not) which are semantically the same but you get different results

I'm not following this either, can you give an example?

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)

But it occured to me that we could prepend a & to the output to make them equivalent again.

Which output?

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 */

Am I understanding you correctly that you rather have the div :global.x -> div.x behavior?

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.

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 :global.x suddenly violates that then that would be very confusing to me - more than appending :global to the end of a selector.

@dummdidumm
Copy link
Member Author

I created #12560 again implementing the "remove preceeding descendant combinator" so we can compare side by side and then make a decision.

@Rich-Harris
Copy link
Member

merged #12560, closing

@Conduitry Conduitry deleted the css-global-tweaks branch July 25, 2024 17:41
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.

Tailwind @apply breaks with Svelte 5 (alpha 57)
4 participants