-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Expose inferRemoteSize
function
#11098
Expose inferRemoteSize
function
#11098
Conversation
🦋 Changeset detectedLatest commit: 934a54f 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 |
There was a problem hiding this 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.
Refactored it a bit, the only thing missing will be adding better error messages now that this is exposed and stuff. I'll do that later, thank you for contributing! |
Thank you, as always, @Princesseuh! |
@itsmatteomanf please ping me when this PR is ready to be shipped. Do you think it will be ready for the next minor? |
@ematipico as far as I am concerned it's ready, I know @Princesseuh had some changes in mind though, but said she was going to do them herself... |
Yeah, will tackle when I'm back from holidays if no one does. The changes required are just making the errors better, now that the feature is exposed. Docs might be great as well! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking this while we figure out what docs are needed!
The current docs for inferSize
are here: https://docs.astro.build/en/guides/images/#infersize so this would be a place to check.
Also checking that we don't need any new error messages associated with making this available to users now?
Additionally, adding the necessary context (why this PR was made, what this allows you to do) inside the PR description rather than linking to a Discord message (not publicly available) is helpful! This would have helped me suggest an appropriate changeset message.
Hmm, okay there's a problem with exporting it from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic example, thank you @Princesseuh ! One quick thought for your consideration, but happy for you to make the final call.
Co-authored-by: Sarah Rainsberger <[email protected]>
@@ -1,4 +1,4 @@ | |||
export { emitESMImage } from './emitAsset.js'; | |||
export { emitESMImage } from './node/emitAsset.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is annoying, emitAsset
(obviously) isn't non-Node safe, but I exported it from the utils without thinking a while ago, and now it means that astro:assets
can't actually import from astro/assets/utils
otherwise it breaks runtime.
For now, I moved emitAsset
to its own folder to reduce damage, and in the future I'll update the Markdoc integration and stuff to import it from the proper place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for Docs!
Co-authored-by: Sarah Rainsberger <[email protected]>
* feat: expose and rename `inferSize` * feat: separate `ISize` type * feat: reformat function to use `ImageMetadata` * nit(assets): re-use image-metadata code for remote images * chore: changeset * chore: changeset * feat(assets): Export from `astro:assets` * fix: proper errors * fix: dont export from astro/assets * fix: ests * Update .changeset/large-geese-play.md Co-authored-by: Sarah Rainsberger <[email protected]> * fix: ests * Update .changeset/large-geese-play.md Co-authored-by: Sarah Rainsberger <[email protected]> --------- Co-authored-by: Erika <[email protected]> Co-authored-by: Sarah Rainsberger <[email protected]>
Changes
This change exposes the old
inferSize()
, renamed toinferRemoteSize()
per https://discord.com/channels/830184174198718474/1240790957591367701.Testing
No additional testing needed, it was never tested directly.
Docs
withastro/docs#8808