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

Consider imports that are not updated or mutated to be constant #8933

Closed
benmccann opened this issue Jul 7, 2023 · 3 comments · Fixed by #8948
Closed

Consider imports that are not updated or mutated to be constant #8933

benmccann opened this issue Jul 7, 2023 · 3 comments · Fixed by #8948
Labels

Comments

@benmccann
Copy link
Member

benmccann commented Jul 7, 2023

Describe the problem

This gets converted to 50 lines of JS:

<picture>
	<source srcset="/_app/immutable/assets/living-carbon-logo.92781a1d.avif 530w" type="image/avif">
	<source srcset="/_app/immutable/assets/living-carbon-logo.181689e7.webp 530w" type="image/webp">
	<img src="/_app/immutable/assets/living-carbon-logo.dae03d6f.png" width="530" height="89" alt="Living Carbon">
</picture>

And it scales really well. If you put 5 pictures inside a <div> you end up with 50 lines of JS. I believe the concise output here is courtesy of #7426 in Svelte 4.0.

On the other hand, this gets converted to 112 lines of JS:

<script>
	const avif = "/_app/immutable/assets/living-carbon-logo.92781a1d.avif 530w";
	const webp = "/_app/immutable/assets/living-carbon-logo.181689e7.webp 530w";
	const img = {
		src: "/_app/immutable/assets/living-carbon-logo.dae03d6f.png",
		w: 530,
		h: 89
	};
</script>

<picture>
	<source srcset={avif} type="image/avif">
	<source srcset={webp} type="image/webp">
	<img src={img.src} width={img.w} height={img.h} alt="Living Carbon">
</picture>

And it doesn't scale nearly as nicely. If you put just 2 pictures inside a <div> you end up with 176 lines of JS. But if we want to use things like Vite's asset handling, we have to write code that looks a lot more like the second.

This ends up being important if we implement the Svelte image tag as a preprocessor as @Rich-Harris has proposed (sveltejs/kit#9787 (comment)). The preprocessor approach doesn't scale as well as an Image component at the moment, but I believe it'd do even better if we made this change.

Describe the proposed solution

I'm hoping it'd be easy at least with the constant primitives. With the constant object you might have to do a bit more work to see that nothing's touching it. If it helps analyze it, we could create those objects wrapped in Object.freeze or something to indicate that they're not modified.

Alternatives considered

Don't get that final point on Lighthouse 😆

Screenshot from 2023-07-06 20-16-20

Importance

nice to have

@benmccann
Copy link
Member Author

Hmm. Thinking about this a bit more... In the case of handling images it's not actually const that we have, but a default import. I guess with an import the Svelte compiler doesn't resolve it and doesn't know if it's an immutable primitive or mutable object or whatnot. It might be hard to handle in the general case, but I think there's a lot we could do to make things easier since we're writing these asset imports and know they're immutable. E.g. similar to /*#__PURE__*/ comment, perhaps we could write the preprocessor to output a new /*#__IMMUTABLE__*/ comment telling Svelte that it's safe to assume it's immutable and optimize it.

@benmccann benmccann added the perf label Jul 7, 2023
@benmccann
Copy link
Member Author

benmccann commented Jul 7, 2023

I realized we wouldn't have to create a new comment like /*#__IMMUTABLE__*/ because we already have /** @readonly */ in JSDoc: https://jsdoc.app/tags-readonly.html

And probably we wouldn't actually inline the constants , but just do string interpolation. I.e. convert it to:

`<picture>
	<source srcset=${avif} type="image/avif">
	<source srcset=${webp} type="image/webp">
	<img src=${img.src} width=${img.w} height=${img.h} alt="Living Carbon">
</picture>`

@benmccann
Copy link
Member Author

I'm thinking a good way to implement this actually would be to have a new compiler option that accepts a regex for determining whether an import is an asset and thus immutable. Then vite-plugin-svelte could provide Vite's DEFAULT_ASSETS_RE to the Svelte compiler.

@benmccann benmccann changed the title Inline constants Consider imports that are not updated or mutated to be constant Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant