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

feat: better errors for images #8536

Merged
merged 4 commits into from
Sep 13, 2023
Merged

feat: better errors for images #8536

merged 4 commits into from
Sep 13, 2023

Conversation

Princesseuh
Copy link
Member

Changes

Users makes some understandable mistakes around using <Image />:

  • They pass filepaths, instead of imports
  • They pass the result of a non-eager import.meta.glob

This PR adds better documentation and error messages around these cases to better inform users on the proper way to use <Image />. Hopefully it'll help!

This PR also includes a small styling change to the overlay, as informed by Mark.

Testing

Tested manually the new messages, we have tests for errors otherwise already

Docs

Some of this is docs!

/cc @withastro/maintainers-docs

@Princesseuh Princesseuh requested a review from a team as a code owner September 13, 2023 11:25
@changeset-bot
Copy link

changeset-bot bot commented Sep 13, 2023

🦋 Changeset detected

Latest commit: 1286af2

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 Sep 13, 2023
const resolvedOptions: ImageTransform = {
...options,
src:
typeof options.src === 'object' && 'then' in options.src
? (await options.src).default
? (await options.src).default ?? (await options.src)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a small behaviour change to support using non-eager import.meta.glob better. Saw some users confused that they couldn't pass the result of it to Image without awaiting multiple times and calling multiple functions in some cases. This was particularly relevant in MDX where you can't await everything the same way as in Astro files

const boldRegex = /\*\*(.+)\*\*/gm;
const urlRegex = /(\b(https?|ftp):\/\/[-A-Z0-9+&@#\\/%?=~_|!:,.;]*[-A-Z0-9+&@#\\/%=~_|])/gim;
const urlRegex = / (\b(https?|ftp):\/\/[-A-Z0-9+&@#\\/%?=~_|!:,.;]*[-A-Z0-9+&@#\\/%=~_|])/gim;
Copy link
Member Author

@Princesseuh Princesseuh Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small regression fix in this regex. I changed it not too long ago and it broke some links in subtle ways, this fixes that

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs look good! Thanks @Princesseuh !

@Princesseuh Princesseuh merged commit b85c8a7 into main Sep 13, 2023
@Princesseuh Princesseuh deleted the feat/better-image-errors branch September 13, 2023 14:44
@astrobot-houston astrobot-houston mentioned this pull request Sep 13, 2023
@Marocco2
Copy link

This PR blocks production on my env as I rely on string based filepath for og:image

Minimum example:

const { title, description, heroImage = "@assets/blog/introducing-astro.jpg" } = Astro.props;
const { src }= await getImage({
src: heroImage, width: 640, height: 426,
alt: ''
});
const socialImageURL = new URL(src, Astro.site);
<meta property="og:image" content={socialImageURL} />

@Princesseuh
Copy link
Member Author

This PR blocks production on my env as I rely on string based filepath for og:image

Minimum example:

const { title, description, heroImage = "@assets/blog/introducing-astro.jpg" } = Astro.props;
const { src }= await getImage({
src: heroImage, width: 640, height: 426,
alt: ''
});
const socialImageURL = new URL(src, Astro.site);
<meta property="og:image" content={socialImageURL} />

How did this work before? Before this PR this would've ended up being an invalid path at build time anyway.

@Marocco2
Copy link

How did this work before? Before this PR this would've ended up being an invalid path at build time anyway.

Not at all. Basically I would pass heroImage from a frontmatter (for ex: "@assets/blog/hello-world.jpg"). getImage() would resolve the path and generate an optimized version of the image which I would use for og:image

@Princesseuh
Copy link
Member Author

Princesseuh commented Sep 18, 2023

How did this work before? Before this PR this would've ended up being an invalid path at build time anyway.

Not at all. Basically I would pass heroImage from a frontmatter (for ex: "@assets/blog/hello-world.jpg"). getImage() would resolve the path and generate an optimized version of the image which I would use for og:image

Hmm, getImage does not resolve images... I'd be curious to see a reproduction, because in my mind (having both made this PR and all of astro:assets), there's no way this worked in prod.

@Marocco2
Copy link

Marocco2 commented Sep 18, 2023

How did this work before? Before this PR this would've ended up being an invalid path at build time anyway.

Not at all. Basically I would pass heroImage from a frontmatter (for ex: "@assets/blog/hello-world.jpg"). getImage() would resolve the path and generate an optimized version of the image which I would use for og:image

Hmm, getImage does not resolve images... I'd be curious to see a reproduction, because in my mind (having both made this PR and all of astro:assets), there's no way this worked in prod.

I'll try to make a min repro in a few days 👍

@Marocco2
Copy link

Alright, I found a better way to deal with this stuff. Sorry for any inconvenience. Keep up the good work 👍

@wddesousa
Copy link

Alright, I found a better way to deal with this stuff. Sorry for any inconvenience. Keep up the good work 👍

can you tell me how? I'm struggling to making og:image work with cover images saved in the src folder but can't figure it out

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.

6 participants