Skip to content

Conversation

@patrickarlt
Copy link

Changes

Testing

See included unit test.

Docs

/cc @withastro/maintainers-docs for feedback!

I'm happy to do a matching PR to the docs if preferred.

@changeset-bot
Copy link

changeset-bot bot commented Nov 29, 2025

🦋 Changeset detected

Latest commit: 2cd2666

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 Nov 29, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 29, 2025

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing patrickarlt:async-file-parse (2cd2666) with main (245a7d4)1

Summary

✅ 9 untouched benchmarks

Footnotes

  1. No successful run was found on main (8068bad) during the generation of this report, so 245a7d4 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

That seems fine to me! Any objection you see @ascorbic perhaps?

@Princesseuh Princesseuh added this to the 5.17.0 milestone Jan 15, 2026
@sarah11918
Copy link
Member

From a docs standpoint, just checking here: is this "allows the option for the parser() function to be async if you add await" or is it "updates the parser() function to require await and always run asynchronously"? And, if it's now updated, could this possibly break anyone's existing use of the parser() function? Is there anything they might have to check for to be sure?

Here's the existing docs for the parser() function that we would need to figure out how this affects, and we'd also have to check for any examples (e.g. in the Content collections guide, astro:assets API, and perhaps elsewhere throughout docs that if we show examples. So we'd need a PR to current live docs that does a sweep just to make sure we haven't got places where we've relied on the old behaviour.

This change is a little tricker than normal because the v6 docs have been significantly overhauled for content collections, and don't match up cleanly with our existing ones. So, I would also ask for a second docs PR made to the v6 branch too, so we can port the changes over smoothly for our revised documentation over there.

Once we've clarified exactly what the impact of this change is, then @ArmandPhilippot and I can help with the changeset, and with suggesting what should happen in docs!

@ArmandPhilippot
Copy link
Member

is this "allows the option for the parser() function to be async if you add await" or is it "updates the parser() function to require await and always run asynchronously"?

Looking at the type, this is "allows" (nothing is required, unless there is an uncaught side effect somewhere in the implementation... but I don't think so, and this would be a bug). So, the user should not need to do any updates and I don't think this requires a lot of updates in docs.

But, yes, we would need to update the Type: on parser() because this is now async. However, because we tend to show only public types (ie. importable)... This would be something like:

(text: string) => Record<string, Record<string, unknown>> | Array<Record<string, unknown>> | Promise<Record<string, Record<string, unknown>> | Array<Record<string, unknown>>>

which is bit long (3 lines on desktop view with my resolution, I think we have at least another type on 3 lines but this is not the most readable unfortunately).

I wonder if it would make sense to have a public type alias for data (unless I'm wrong and this doesn't match data?). We could keep Record<string, unknown> under the data section and have something like CollectionData (a bit shorter, or something similar but shorter than Record<string, unknown>) for the other types. This would allows us to have something like:

(text: string) => Record<string, CollectionData> | Array<CollectionData> | Promise<Record<string, CollectionData> | Array<CollectionData>>

Which I think improves the readability. And we could use a link on CollectionData for those who would like to see the full type.

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.

4 participants