Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add experimental
@sveltejs/image
package #9787feat: add experimental
@sveltejs/image
package #9787Changes from 10 commits
9d83a60
5716ed4
fab9aec
8eac4b5
f29c9f6
7b2d891
59a528d
9971ab7
1a16981
3af5be8
400942d
778fc7e
73cdf53
49c06cb
cbc25b2
c0a6eec
5a68b43
2cdb3bf
7c7b2a6
f3bb601
b5bbb13
d0e30bd
bfb7068
d23bd24
4293d7c
0e07ee2
81968ae
c8afcf8
b2a288b
16783a3
e1c89be
0b3105b
8040287
aef41b1
438c64d
a54237b
4aa0891
fab06e5
463a9da
f83793b
c4a9fa8
c50c3cf
9725200
a13ff82
c6f1695
8e02170
cd72904
3593bca
a1eb549
f991577
12ed8a9
ccd4e2b
7412d29
3c4fa50
848b450
6e2168a
365252f
959180e
1bbe9bd
77e7349
ea3cb41
fe130a2
81fbbde
afab1fb
27a9e1e
a2c8f4f
6bca660
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason not to enable this by default? will anything bad happen if you do and don't have
@sveltejs/image
installed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't know, but it feels wrong to automatically enable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cannot be cached. If you
import
it then Vite will compute a hash giving it a unique URL that you can set far forwards expires headers one. Importing also allows Vite to inline small images. I think we should still encourage images to be imported for dynamic CDNs.You could set a project-level default for static vs dynamic and then opt-in/out with a query parameter that triggers the
include
/exclude
ofvite-imagetools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
".. that you can set far forwards expires headers one" - I don't understand what you mean by that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"one" was a typo and was supposed to be "on". What I mean is setting
'cache-control': 'public, immutable, max-age=31536000'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we talked this morning you said
/path/to/your/image.jpg
is something that would probably not exist locally. Do we need some documentation around how to deploy it? Or maybe some best practices like creating an/immutable
path that can be cached and images outside of that would not be cached or something? Maybe with examples for the built-in providers?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow. What I meant is that if your
src
path starts with a/
or withhttp(s)://
it indicates that you're not importing some local image file on disk but instead want to reference something through the URL. What that means with regards to caching depends on where the image is stored. I wouldn't worry about that for now, we can document that later (since it's only a documentation thing, not an implementation issue).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using
/favicon.ico
resolves to/static/favicon.ico
locally. so it could be confusing if some paths lead to local sources and others to a cdn url. Would an alias work? eg.$cdn/some-image.png
where $cdn is a configured value from svelte/image/vite ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
problem solved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how I feel about this... I kinda feel like things like this would be better expressed using slots, so we can also use events, actions etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like
feels better to me at first. But it probably does not work with the static import preprocessor then..?
Other cons: If you refactor from one to the other it's more stuff to do for you with the
getImage
versionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... this is a super shitty thing to say after so much work has gone into the component so far, but I'm using it and my main thought is why is this a component?. I want to use the
smoothload
action, and I can't, and it just feels like this would be so much simpler than dealing with a component:There's no magic, it's way easier to understand what's happening, it's not switching between
<img>
and<picture>
based on factors that I don't fully understand, etc.Maybe our mistake was trying to solve build time and runtime optimizations in the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I don't think refactoring from one to the other is something we need to worry about — it's very unlikely that you'd need to do that, because they're really just different problems that happen to both involve an
<img>
as part of the solution)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a common case is that users will start with the build-time solution and then outgrow it and move to a CDN. The build-time solution is much easier to setup - it doesn't require setting up a CDN account, etc. But as sites grow I imagine many people will end up migrating to a CDN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build time to me means things like icons; run time means things like product shots, which you'd never have in your
src
folder in the first place (except maybe for some very early prototyping that you later replace with real data, which isn't the same as 'outgrowing')There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a user perspective (indie SaaS & small startups), I'd prefer to stick with build-time generated images as long as practical speed wise--which could be a while for an indie SaaS with 3-4 marketing pages and low-volume blog.
Being build-time static means 1.) fewer knobs to get off the ground, and 2.) better guarantees that a DDoS will cost nothing on hosts that provide zero-cost static file serving, which wouldn't be the case with a misconfigured cache, etc.
A couple reasons to stay static as long as practical for some devs.