Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Pass remote Markdown images through image service #13254

Merged
merged 17 commits into from
Feb 26, 2025

Conversation

p0lyw0lf
Copy link
Contributor

@p0lyw0lf p0lyw0lf commented Feb 15, 2025

Changes

  • Currently, in Markdown files, only local images get passed to the image service
    • This was because the image service only supported local files when it was first released.
  • However, the image service now supports remote images
  • So, this PR adds support for passing remote images in Markdown files to the image service.

Testing

I tested this change by creating a small project with a single allowed remote domain, and rendering markdown files from:

  • a page
  • a content collection

Both approaches work. I can provide these test files if needed, though I'm not sure how we'll want to deal with the fact it'll require an internet connection.

Known Issues

If the remote image is styled in some way, the newly added width and height attributes could mess up that styling. Ideally, we'd like to be able to modify the properties after getImage runs somehow, but seeing as this is also the case for local images transformed using this method too, I think this is more a deficiency in getImage best left addressed by the responsive image experiment.

This is only an issue for remote images as configured under images.domains. Other images are untouched.

Docs

If this change goes through, we will need to update the images guide to reflect that remote images are now supported in markdown files. PR: withastro/docs#11021

When in regular .md files, the images show up as expected. However, in content md files, they still misbehave. Also, it seems the responsive attributes are only added by the <Image> component, not the image transformation that's done later, which I find a bit weird; there's other responsive stuff being done tho?? So maybe I need to fix that while I'm here too.
Copy link

changeset-bot bot commented Feb 15, 2025

🦋 Changeset detected

Latest commit: a21e0b5

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 feat: markdown Related to Markdown (scope) pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) labels Feb 15, 2025
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Feb 15, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

Copy link

codspeed-hq bot commented Feb 15, 2025

CodSpeed Performance Report

Merging #13254 will not alter performance

Comparing p0lyw0lf:markdown-collect-remote-image (a21e0b5) with main (2e1321e)

Summary

✅ 6 untouched benchmarks

This time the fail was becase we were transforming too many remote images. We should inject allowedRemoteDomains into the remark plugin so we only collect the allowed remote images in the first place.
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Thank you @p0lyw0lf for your PR! I left some comments and questions around the new feature

This commit moves remote pattern checking functionality into @astrojs/markdown-remark, out of the main astro package, so it can be used by remark-collect-images. We do this instead of moving it into a shared @astrojs/internal-helpers because part of that package uses node APIs, and remark-collect-images needs to work in a browser.
@p0lyw0lf
Copy link
Contributor Author

@ematipico I believe I've addressed your feedback. One of the bigger changes was moving the checks for https://docs.astro.build/en/guides/images/#authorizing-remote-images out of astro and into @astrojs/markdown-remark, so as to not duplicate them.

@p0lyw0lf p0lyw0lf requested a review from ematipico February 18, 2025 14:42
This time, the problem is certain parts of `astro` don't like depending
on `@astrojs/markdown-remark`, because it brings in `node:fs` somehow.
This on its own is probably a problem, but I think I'll work around it
by moving the relevant shared code into
`@astrojs/internal-helpers/remote` so that we can stay node-free.
@p0lyw0lf
Copy link
Contributor Author

The fixes I pushed this morning were not enough; there was a error where moving isRemoteAllowed into @astrojs/markdown-remark and including in astro caused node:fs to show up in the module path somehow. I fixed this by moving that code into @astrojs/internal-helpers instead. There was also a small error with that move in the first place, which I have also fixed. Tests pass locally now, hoping they pass here too.

@ematipico
Copy link
Member

Since you added code to another package, that package must have a changeset. Without changeset it won't have its version bumped and it will break in production.

It should be a new changeset, not the one you currently have

@p0lyw0lf
Copy link
Contributor Author

@ematipico I've updated the changeset

@ematipico ematipico added this to the 5.4.0 milestone Feb 21, 2025
@ematipico
Copy link
Member

@p0lyw0lf Thank you for you PR. Coding-wise, it looks good to me. Would you be able to send a PR to docs? If not, that's fine, we can do that. Just let us know. We aim to release the new feature next Thursday.

@p0lyw0lf
Copy link
Contributor Author

p0lyw0lf commented Feb 22, 2025

@ematipico I opened a docs PR: withastro/docs#11021

Also, any notes on #13256 by any chance?

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.

Exciting feature! 🚀

I left my thought on the changeset for docs, and Ema can guide you on creating individual ones that describe what's new / changes about the other packages.

I've also reviewed the docs PR which won't take much to polish it up! 🙌

(Also noting, I didn't notice any new error messages associated with this feature.)

p0lyw0lf and others added 2 commits February 24, 2025 20:57
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
@p0lyw0lf
Copy link
Contributor Author

@sarah11918 @ematipico I've split the changeset into multiple, adding specifics on what's changed for each package. Thanks again for reviewing this!!

p0lyw0lf and others added 4 commits February 25, 2025 11:49
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
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.

Looks good to me! Approving for docs! 🥳

(And noting that I dont see any new error messages introduced with this feature)

@ematipico ematipico merged commit 1e11f5e into withastro:main Feb 26, 2025
17 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Feb 26, 2025
@aholbreich
Copy link

aholbreich commented Feb 28, 2025

Has anything changed on the usage side?

having

---
cover: https://remote-cdn.io/cool.jpeg
---

would still work?
Btw. I remember to see some warnings about width in those cases..

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
docs pr feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants