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

fix: take snippets into account when scoping CSS #13589

Merged
merged 3 commits into from
Oct 15, 2024
Merged

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Oct 13, 2024

Updated and finalized version of #10208.

The approach is to collect all render tags, then for each render tag both traverse up the elements and cut selectors from the end, until either no options left or a match is found. The assumption behind this is that the contents of the render tag would complete the selector.

There are ways to make this more strict (cutting a selector from the end only if the combinator still can match afterwards, check if we have snippets in the component and where they are rendered, etc), but the result will only mean more things pruned potentially, but no false negatives, so this is a good first iteration IMO.

Ideally merged after #13567/#13568 since it will result in some merge conflicts and this PR is far simpler to resolve the conflicts within

fixes #10143

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 Oct 13, 2024

🦋 Changeset detected

Latest commit: 1b693c8

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

let element = context.state.element;

while (element) {
const selectors_to_check = selectors.slice();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we traverse backwards through the selectors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wdym? We're traversing them back to front. That's what the rest of the pruning algorithm does.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean a for loop with negative index rather copying the array and popping each item

Copy link
Member Author

Choose a reason for hiding this comment

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

The result of pop isn't passed to anything, the resulting array that is one item less afterwards is. Looping with negative index wouldn't change the fact that we have to create a new array, if that's what you're concerned about (in fact we would have to create more since we'd have to do it for each iteration of the inner loop, rather of only each iteration of the outer loop)

@dummdidumm dummdidumm merged commit 829be3d into main Oct 15, 2024
9 checks passed
@dummdidumm dummdidumm deleted the snippet-css-scopes branch October 15, 2024 14:22
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.

Svelte 5: Nested styles for snippets - scope issue
2 participants