Skip to content

Conversation

@ematipico
Copy link
Member

Changes

This PR adds more JsDoc to the utility functions that we expose for image services.

I used a LLM tool to generate the docs, and then reviewed them to make sure they seem correct.

I also noticed resolveSrc was awaiting twice the same resource in case no .default wasn't returned. I changed the code so the awaiting is done once every time.

I also slightly refactored imageMetadata because we were throwing an error inside a try block.

Testing

CI should stay green

Docs

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Mar 5, 2025

🦋 Changeset detected

Latest commit: c305ef2

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 Mar 5, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 5, 2025

CodSpeed Performance Report

Merging #13367 will not alter performance

Comparing docs/asset-utils (c305ef2) with main (d83f92a)

Summary

✅ 6 untouched benchmarks

Copy link
Member

@ArmandPhilippot ArmandPhilippot left a comment

Choose a reason for hiding this comment

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

LGTM! I only left a few non-blocking nits.

}

const { width, height, type, orientation } = result;
const isPortrait = (orientation || 0) >= 5;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's just my lack of knowledge, but why 5? Shouldn't there be a comment to explain this number?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you, however I don't know the code either. Maybe @Princesseuh knows

Co-authored-by: Armand Philippot <[email protected]>
@ematipico ematipico merged commit 3ce4ad9 into main Mar 10, 2025
10 of 15 checks passed
@ematipico ematipico deleted the docs/asset-utils branch March 10, 2025 08:39
@astrobot-houston astrobot-houston mentioned this pull request Mar 10, 2025
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.

2 participants