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: support Astro _image endpoint #94

Merged
merged 4 commits into from
Dec 5, 2023
Merged

feat: support Astro _image endpoint #94

merged 4 commits into from
Dec 5, 2023

Conversation

jlarmstrongiv
Copy link
Contributor

Closes #93

Copy link
Owner

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for this. We just need a few things to get this ready to merge.

As well as the individual comments, framework transformers need to implement delegateUrl. This is needed so that if the user passes a URL with an href that matches another CDN, we use that instead of the local server. Take a look at the Next.js tests for examples.

src/transformers/astro.test.ts Show resolved Hide resolved
src/transformers/astro.ts Outdated Show resolved Hide resolved
@jlarmstrongiv
Copy link
Contributor Author

Hey! Thanks for this. We just need a few things to get this ready to merge.

As well as the individual comments, framework transformers need to implement delegateUrl. This is needed so that if the user passes a URL with an href that matches another CDN, we use that instead of the local server. Take a look at the Next.js tests for examples.

Sure! I think the delegateUrl function that next and vercel uses looks good to me, so I moved it to utils and used it for astro too

@jlarmstrongiv
Copy link
Contributor Author

Just wanted to say thank you for your help reviewing this PR @ascorbic 👍 great suggestions

src/transformers/astro.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
@jlarmstrongiv
Copy link
Contributor Author

I fixed those issues and squashed to a single commit 👍

@jlarmstrongiv
Copy link
Contributor Author

Hi @ascorbic 👋 please let me know if there’s anything else I need to fix with this PR—thanks again for reviewing 👍

@jstormail2
Copy link

@ascorbic and @jlarmstrongiv are there any blockers for this PR? I found unpic-img has an astro component, and wanted to use it with the astro assets image endpoint in my project

Copy link
Owner

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Thanks!

@ascorbic ascorbic merged commit b015190 into ascorbic:main Dec 5, 2023
6 checks passed
@mixie-bot mixie-bot bot mentioned this pull request Dec 5, 2023
@jlarmstrongiv
Copy link
Contributor Author

Thank you @ascorbic ! Your suggestions were very helpful 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: support Astro transformer
3 participants