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: implement support for :is(...) and :where(...) #10490

Merged
merged 16 commits into from
Feb 17, 2024
Merged

feat: implement support for :is(...) and :where(...) #10490

merged 16 commits into from
Feb 17, 2024

Conversation

Rich-Harris
Copy link
Member

This PR implements proper support for :is(...) and :where(...).

In Svelte 4, these are ignored for scoping purposes, like other pseudo-classes that aren't :global(...). The outcome is that a selector like this...

.foo :is(.bar *, .unused) {...}

...becomes this:

.foo.svelte-xyz123 :is(.bar *, .unused) {...}

This is slightly unhelpful:

  • it will match descendants of any class="bar" element, not just those defined within the component. That might not be what you want
  • the .unused classname is kept, even though it's... unused

As of this PR, we get this instead:

.foo.svelte-xyz123 :is(.bar:where(.svelte-xyz123) :where(.svelte-xyz123), /* (unused) .unused*/) {...}

This behaviour is more desirable. If you do want the old 'match any class="bar" descendant' behaviour, you can easily achieve that with :global(...):

.foo :is(:global(.bar) *, .unused) {...}

Implementing this is a necessary precursor to nested CSS support, since it involves the same recursive mechanism.

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 Feb 15, 2024

🦋 Changeset detected

Latest commit: 094f2b1

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

@Rich-Harris Rich-Harris mentioned this pull request Feb 16, 2024
5 tasks
* parse nested CSS

* tests

* track parent rules

* some progress

* switch it up

* pruning

* works

* changeset

* lint

* error early on invalid nesting selector

* tidy

* note to self

* fix some specificity stuff

* failing test

* note to self

* fix: correctly scope CSS selectors with descendant combinators (#10502)

* fix traversal, but break some other stuff

* man this is fucken hard

* fixes

* getting closer

* be conservative for now

* tidy

* invert

* invert

* simplify

* switch

* for now

* progress

* i think it works?

* fix

* tidy up

* revert some stuff

* remove some junk

* handle weird cases

* update

* tweak

* shrink

* changeset

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>
* @param {import('#compiler').Css.ComplexSelector} node
*/
function truncate(node) {
const i = node.children.findLastIndex(({ metadata }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that i might be -1 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and that's fine because we do array.slice(0, -1 + 1) which gives us an empty array, which is exactly what we want

}

return false;
default:
// TODO other combinators
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to throw an error if we're missing a combinator?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's better to just be conservative and include the CSS — removing it is an optimisation. The only combinator we're missing is ||. If another combinator is added to the language, all we need to do (until we have time to implement pruning for it, which may be prohibitively difficult) is adjust a regex

if (child.type === 'Rule' && is_used(child)) return true;

if (child.type === 'Atrule') {
return true; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this TODO mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

that we don't yet analyse atrules to determine whether they are used in the context of the current component. it was there before this PR but in a different place

playgrounds/sandbox/run.js Outdated Show resolved Hide resolved
Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

Added some comments. Overall, LGTM.

@Rich-Harris Rich-Harris merged commit 5605e8c into main Feb 17, 2024
6 of 8 checks passed
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.

None yet

3 participants