Suggestion for an easier to use image component #447
Replies: 7 comments 3 replies
-
Inferring the width / height for remote images using fetch would be nice upgrade. Currently I've built my own image component to do that. |
Beta Was this translation helpful? Give feedback.
-
@Princesseuh this is a great start. The idea of local/remote does seem to be confusing. Is there an argument to be made not to support "remote" images that are in fact local? Why not use statically imported? Or is there a use-case for the local-in-public images that I'm not seeing? |
Beta Was this translation helpful? Give feedback.
-
I'd be curious to see some performance benchmarks for this kind of approach 👀 It looks like |
Beta Was this translation helpful? Give feedback.
-
After spending some time on this component. I think I have an idea of a solution that could help to disambiguate local versus remote more concretely. Local can be referred to by either of the two following URLs All others are treated as remote: I wanted to make this distinction more clear because in the code there's some checking and branching behind the scenes, for trying to determine this, but a rules based approach could make the code simpler. Is there any situation where a string without a protocol that isn't |
Beta Was this translation helpful? Give feedback.
-
I fully agree with the proposal, at the moment it is confusing to require different props depending on how a "local" image is referred to. Since we are talking about breaking changes in the image integration, I would like to raise another topic however, that I summarized here: withastro/astro#4800 (comment) I first added the However, withastro/astro#4800 changed this behavior (I just noticed it today when upgrading to astro v2) to only perform the background replacement if the format doesn't support alpha layers. This was a breaking change that I would like to revert. Users wanting to keep the alpha layers should simply not use the Whenever appropriate, I would like to propose a change that would look like this to restore the old behavior: beeb/astro@7a78ce6 |
Beta Was this translation helpful? Give feedback.
-
Superseded by #468 Thank you everyone for sharing your feedback! |
Beta Was this translation helpful? Give feedback.
-
This happened. |
Beta Was this translation helpful? Give feedback.
-
Proof of concept implementation PR of this proposal: withastro/astro#5943
Summary
According to our users, the image integration is CCC: Cumbersome, confusing and complicated to use. It's unclear for users why certain props are required, how the different properties impact the resulting image(s) or how the background behavior can differs massively depending on seemingly inoffensive changes to their code.
This proposal aims to outline a plan to make the integration overall easier to use and easier to understand
Background & Motivation
We very often get issues and support threads about the image integration, users claiming it to be buggy when many times, it just turns out that the user misunderstood how to achieve the specific result they were aiming for.
While we could make the current behavior more clear through better error messages, better documentation, better types etc, and it would massively improve the experience already; I believe we can also improve the core experience through the following changes:
Easier nomenclature from the perspective of the user
While it makes a lot of sense from the technical perspective, from the point of view of the user, it is unclear why an image they'd refer to it like such:
<Image src="../assets/my_image.png" />
would be considered to be "remote". As such, I propose the following changes:public
should be considered a "local image"Reduce the number of needed props in the more common cases
For statically imported images, nothing changes. None of the props were required in the first place.
Currently, for dynamically referred local images,
width
,height
(andaspectRatio
, if both were not specified), andformat
are required props.width
,height
(and as suchaspectRatio
), andformat
from them, removing the need for user to manually pass those themselves.src
isn't available at runtime. (Referring to an image fromsrc
in SSR should probably be an error pending a better assets story in Astro)For remote images the same thing applies:
width
,height
(andaspectRatio
, if both were not specified), andformat
are required props. In both SSG and SSR, it is possible for us to fetch the image and infer all the required props.Performance concerns
Most of this inferring business happens during the page rendering (since we need the inferred data to properly fill the
width
andheight
attributes to avoid CLS), this can slow down the page rendering. In both SSG and SSR, this is not a real problem for either statically imported or dynamically referred files, if reading a single file from the file system is slow, we probably have bigger problems.For remote images, this is a more real concern, because it means that to render the page, a
fetch
call has to finish. In SSG, this is annoying, but not a real problem: You need thefetch
to finish eventually to build the page. In SSR however, this means delaying the page render.Possible solutions:
In all cases (statically, dynamic, SSG and SSR), caching is possible, so ultimately this can delay a single request at worse.
Double fetching is not a concern, as the image will either be cached in the HTTP cache or through a filesystem-based one.
Goals
Non-goals of this proposal
In order to keep this proposal simple, the following (understandably commonly-requested) features are not included in this proposal
Example
Similar behaviour is to be expected in all cases, no matter the loader used or the rendering technique (SSG or SSR). As such, these examples will only cover a single case to showoff behaviour
Code
Old behaviour
New behaviour
width
automatically inferred from source image, results in 700x300, keeping original aspect ratioResult HTML
Beta Was this translation helpful? Give feedback.
All reactions