fix: consider static attributes that are inlined in the template #14249
Conversation
🦋 Changeset detectedLatest commit: 88b1115 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 |
|
preview: https://svelte-dev-git-preview-svelte-14249-svelte.vercel.app/ this is an automated message |
|
packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js
Outdated
Show resolved
Hide resolved
| return false; | ||
| // if it's not a text attribute but it's an inlinable expression | ||
| // we will inline it in the template so we can still consider it static | ||
| return is_inlinable_expression( |
There was a problem hiding this comment.
Returning here with true could lead to bugs down the road in case a condition afterwards would return false
There was a problem hiding this comment.
But the subsequent checks are only for static text attributes right?
There was a problem hiding this comment.
Uh i guess except the last one which is about the node name itself...let me see what are we doing in that case
There was a problem hiding this comment.
Oh but actually i'm stuped because the rest only checks for the name so it can be problematic there too
There was a problem hiding this comment.
I moved this check last and moved the check for node.name outside the for loop since it didn't had to be inside the loop
There was a problem hiding this comment.
Oh but wait it's not all actually...because i need to loop over every attribute before returning true
There was a problem hiding this comment.
Ok now it's fixed for real...i wonder if we should add some snapshots for this case to prevent easy regressions
There was a problem hiding this comment.
Let's add some snapshot tests, this code is very fragile without.
packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js
Show resolved
Hide resolved
packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js
Outdated
Show resolved
Hide resolved
packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js
Outdated
Show resolved
Hide resolved
Closes #14241
cc. @benmccann if you want to try the output in the playground
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.Tests and linting
pnpm testand lint the project withpnpm lint