fix: better account for render tags when pruning CSS #14456
Conversation
🦋 Changeset detectedLatest commit: 8e3a529 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 |
|
packages/svelte/src/compiler/phases/2-analyze/visitors/shared/component.js
Outdated
Show resolved
Hide resolved
|
|
||
| let resolved = true; | ||
|
|
||
| for (const attribute of node.attributes) { |
There was a problem hiding this comment.
probably not really relevant, but this iterates over the attributes, and below we're iterating over them again, feels a bit wasteful - but merging them into one probably makes everything harder to reason about
packages/svelte/src/compiler/phases/2-analyze/visitors/shared/component.js
Outdated
Show resolved
Hide resolved
packages/svelte/src/compiler/phases/2-analyze/visitors/RenderTag.js
Outdated
Show resolved
Hide resolved
packages/svelte/src/compiler/phases/2-analyze/visitors/RenderTag.js
Outdated
Show resolved
Hide resolved
…ag.js Co-authored-by: Simon H <[email protected]>
|
@Rich-Harris I wonder if you think a children's reference would help this situation or not? |
|
I don't know what that means? |
What I mean is to make a mechanism to track the children, instead of treating them like black boxes. Similar to how react treats children and allows manipulating them. |
|
No that wouldn't help. You're talking about a runtime-based solution for something that happens at compile time |
Oh! Gotcha, thanks for the correction and sorry for the slight derailment. |
|
This fix that got into svelte I had to revert to svelte The component is huge and with many dependencies, I don't know how to produce a small REPL to reproduce the error. |
It's very simple: delete some code. Did the error go away? Put it back. Rinse and repeat until you've isolated the problem. This technique applies generally. |
|
Should this be the case? Or am I missing something? My existing styles are being pruned, blocking an update. <!-- src/routes/+page.svelte -->
{#snippet nav()}
<nav>
<a href="/">Home</a>
</nav>
{/snippet}
{@render nav()}
<style>
/* Unused CSS selector "nav a"(css_unused_selector)eslintsvelte/valid-compile */
/* Unused CSS selector "nav a"svelte(css_unused_selector) */
nav a {
color: red;
}
</style>Reproducible in the REPL as well. |
|
The maximum call stack issue is likely fixed in the latest version, please try again after updating |
|
This persists on Created a separate issue regarding this issue: |
|
@dummdidumm now it works (using |
Fixes #14399
We currently get both false positives (CSS incorrectly assumed to be used) and false negatives (CSS incorrectly assumed to be unused) around render tags at present. We treat render tags as something of a black box, causing false positives which are harmless but undesirable. The false negatives are much worse.
This PR works by linking snippets to render tags. In many cases this is straightforward, because they're identified by name — if we have a
{#snippet foo()}and a{@render foo()}in the same component, they are unambiguously linked. Similarly if we have a{@render children()}andchildrenis a prop, we know that the render tag doesn't correspond to any markup inside the component, and so we don't need to consider it while pruning. Imports are treated the same way as props.Where it gets more complicated is around render tags that can't be trivially statically analysed, and components. In a case like this...
{#each items as item} {@render item.snippet()} {/each}...or this...
...it's impossible to know which snippets will get rendered without more sophisticated static analysis, so in these cases we deopt and assume that the render tag/component could be rendering any locally defined snippets. In practice, this means there will be occasional false positives, but it should eliminate false negatives.
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint