perf: inline module variables into template #13075
Conversation
🦋 Changeset detectedLatest commit: 346a8c5 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/tests/snapshot/samples/inline-module-vars/_expected/client/index.svelte.js
Show resolved
Hide resolved
benmccann
force-pushed
the
inline-vars
branch
from
63a946a
to
1d2f605
Compare
benmccann
force-pushed
the
inline-vars
branch
from
1d2f605
to
cd8b708
Compare
packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js
Outdated
Show resolved
Hide resolved
packages/svelte/src/compiler/phases/3-transform/client/utils.js
Outdated
Show resolved
Hide resolved
packages/svelte/src/compiler/phases/3-transform/client/utils.js
Outdated
Show resolved
Hide resolved
packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js
Show resolved
Hide resolved
| import "svelte/internal/disclose-version"; | ||
| import * as $ from "svelte/internal/client"; | ||
|
|
||
| const __DECLARED_ASSET_0__ = "__VITE_ASSET__2AM7_y_a__ 1440w, __VITE_ASSET__2AM7_y_b__ 960w"; |
There was a problem hiding this comment.
are these later inlined completely? it should be possible to have one static string here instead of interpolation
There was a problem hiding this comment.
I agree, but that's a bit more complicated to handle here and we've generally tried to avoid adding such complication here in favor of letting general purpose optimizers handle it. Unfortunately esbuild does not yet handle this case, but I've filed a feature request for it: evanw/esbuild#3570. Oxc also does not handle it, so I've file a request there as well so that it will be handled when switching to rolldown, but it was unfortunately closed for the time being: oxc-project/oxc#2646
There was a problem hiding this comment.
so in lieu of external tools supporting it, do we reconsider inlining these ourselves? it would make svelte output more svelte after all.
There was a problem hiding this comment.
potentially as a future enhancement if folks agree. i'm still hoping esbuild and oxc will add support. they haven't said they're against it, but just haven't prioritized it. I'd rather not try to do it as part of this PR though to keep things simple for now as this solves 95% of the issue
|
Before merging it, Simon updated #13132, so that it didn't revert your PR but implemented the more sophisticated fix you're talking about |
Uh completely missed the discussion, sorry |
benmccann
force-pushed
the
inline-vars
branch
from
8595e05
to
77cff50
Compare
packages/svelte/src/compiler/phases/3-transform/client/types.d.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
If @dominikg is ok with moving the enhancement to a later point this LGTM!
|
separate PR is fine, but it should be on the short list. these extra consts and the interpolation are not for free and we should strive to merge as much as possible into static strings |
|
I've filed an issue to track it: #13233 |
ref #12995
helps with sveltejs/kit#12627
based off #10015, but this one only does inlining and no hoisting