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

fix: improve error for inferSize and Image component #11823

Merged

Conversation

DerTimonius
Copy link
Contributor

Changes

This PR improves the error message a user will see when using inferSize in the Image component when the image is hosted locally instead of remote.

The original error message described in the issue (#11821) did not come from Astro, but from fetch. Therefore I figured that the check if the image is a remote image or not was incorrect.

So I added the check for isRemotePath which now triggers the correct error.

Testing

I tested this locally by changing an <img> in the examples/portfolio about.astro page to the <Image> and added inferSize. After the change, the error was the expected error.

Docs

No change to the docs needed

closes #11821

Copy link

changeset-bot bot commented Aug 23, 2024

🦋 Changeset detected

Latest commit: 63fcf6f

The changes in this PR will be included in the next version bump.

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 23, 2024
@Princesseuh
Copy link
Member

Princesseuh commented Sep 3, 2024

Hmm, I'm not sure about this. This function is used for other things than just checking for http paths. I assume that adding this check makes it so it report that width and height are needed? I think it'd perhaps be a better option to add an error for this specific case.

@DerTimonius
Copy link
Contributor Author

@Princesseuh no, the check is for showing the user the correct error when inferSize is being used on a local image. the original error shown in the issue did not come from Astro, but was bubbled up by fetch as it's not provided a valid URL.

if I understand the code correctly, the check that triggers the fetch call is this:
image

so if you think that the check should not be in the isRemoteImage function, I would move it to this if block instead

@Princesseuh
Copy link
Member

Yes, I understand the goal. However the isRemoteImage function is used in other parts of the code than just this specific check. What your change does is invisibly ignore inferSize when the image is not an actual URL, but I think it'd produce a better experience if we instead had an error that would tell the user explicitly that inferSize cannot be used with images inpublic

@DerTimonius
Copy link
Contributor Author

I have checked if the desired error (which I guess is the one in the screenshot) is displayed with this change by adapting one of the examples (a good place would be the about page in the portfolio example)

image

this is the code that triggered this error:
image

@DerTimonius
Copy link
Contributor Author

@Princesseuh I moved the check for isRemotePath to where I think it should fit if it should not be added to the isRemoteImage check. Would this meet your expectations or should I create a new error?

@Princesseuh
Copy link
Member

I think this is better! Ultimately, we'd probably want it done completely differently, but that's something to think through outside of this PR. Thank you!

@Princesseuh Princesseuh merged commit a3d30a6 into withastro:main Oct 16, 2024
4 checks passed
@DerTimonius DerTimonius deleted the 11821-inferSize-error-local-image branch October 16, 2024 10:48
@astrobot-houston astrobot-houston mentioned this pull request Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getImage and Image return Failed to parse URL from when inferSize is set to true
2 participants