Skip to content

feat: move astro config validation after astro:config:setup#13482

Merged
ematipico merged 23 commits intomainfrom
feat/post-integrations-config-validation
Apr 2, 2025
Merged

feat: move astro config validation after astro:config:setup#13482
ematipico merged 23 commits intomainfrom
feat/post-integrations-config-validation

Conversation

@florian-lefebvre
Copy link
Copy Markdown
Member

@florian-lefebvre florian-lefebvre commented Mar 21, 2025 •
edited
Loading

Changes

  • In the config schema, refine and superRefine were only called when validating the user's config
  • With this PR, those validations are moved to another schema, called for the user config (as before) as well as at the end of astro:config:setup for each integration
  • Config related schemas are split in several files for clarity
  • Integrations hooks are refactored
  • Unhandled errors in integration hooks now log which integration and which hook is failing
  • I recommend you review the PR without whitespace changes

Testing

Updated, should pass

Docs

Changeset, no docs update needed I think

@florian-lefebvre florian-lefebvre self-assigned this Mar 21, 2025
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 21, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: ef18425

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 21, 2025
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Mar 21, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #13482 will not alter performance

Comparing feat/post-integrations-config-validation (ef18425) with main (b8645c1)

Summary

✅ 6 untouched benchmarks

@github-actions github-actions Bot added the semver: minor Change triggers a `minor` release label Mar 21, 2025
github-actions[bot]
github-actions Bot previously requested changes Mar 21, 2025
View reviewed changes
Copy link
Copy Markdown
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.

@florian-lefebvre florian-lefebvre marked this pull request as ready for review March 21, 2025 12:06
@florian-lefebvre florian-lefebvre marked this pull request as draft March 21, 2025 14:12
@florian-lefebvre florian-lefebvre mentioned this pull request Mar 26, 2025
Merged
@florian-lefebvre florian-lefebvre marked this pull request as ready for review March 26, 2025 12:32
@florian-lefebvre florian-lefebvre added this to the v5.6.0 milestone Mar 26, 2025
ematipico
ematipico reviewed Mar 26, 2025
View reviewed changes
Copy link
Copy Markdown
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.

I believe the new names are misleading. Calling the new schema refined isn't correct IMHO, unless I misunderstood its intent. Now that we have a schema that runs after the hooks, we can also run transformations, right? Without affecting downstream integrations.

If so, can we call the new schema post validation or something similar?

Comment thread packages/astro/src/core/config/schemas/README.md Outdated
@florian-lefebvre @ematipico
Update packages/astro/src/core/config/schemas/README.md
57c55bd 
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
@florian-lefebvre
Copy link
Copy Markdown
Member Author

florian-lefebvre commented Mar 27, 2025

@ematipico it's called refined on purpose because it only performs a set of checks inside of .superRefine(), there are no transformations happening. I think if we want to be able to do transforms before being stored in settings, that will require another PR (because eg. types will have to added/renamed across a lot of places etc)

@ematipico
Copy link
Copy Markdown
Member

ematipico commented Mar 27, 2025

Ok, I thought the initial intent of the PR was to allow transformations after validation.

delucis
delucis reviewed Mar 27, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thanks for the ping @florian-lefebvre! Left some comments.

Comment thread .changeset/easy-vans-laugh.md
Comment thread packages/astro/src/core/config/schemas/README.md Outdated
Comment thread packages/astro/src/integrations/hooks.ts
sarah11918
sarah11918 reviewed Mar 28, 2025
View reviewed changes
Comment thread .changeset/easy-vans-laugh.md Outdated
sarah11918
sarah11918 reviewed Mar 28, 2025
View reviewed changes
Comment thread .changeset/easy-vans-laugh.md Outdated
sarah11918
sarah11918 reviewed Apr 1, 2025
View reviewed changes
Comment thread .changeset/easy-vans-laugh.md Outdated
sarah11918
sarah11918 approved these changes Apr 1, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Approving for docs!

@ematipico
Copy link
Copy Markdown
Member

ematipico commented Apr 2, 2025

@delucis, I am going to merge this PR. Please let us know if you have concerns.

@ematipico ematipico merged commit ff257df into main Apr 2, 2025
17 checks passed
@ematipico ematipico deleted the feat/post-integrations-config-validation branch April 2, 2025 10:09
@astrobot-houston astrobot-houston mentioned this pull request Apr 2, 2025
Merged
@delucis delucis mentioned this pull request Apr 3, 2025
Closed
openscript pushed a commit to openscript/astro that referenced this pull request Sep 12, 2025
@florian-lefebvre @ematipico
@delucis @sarah11918 @openscript
feat: move astro config validation after astro:config:setup (withastr…
1561a85 
…o#13482)

* feat: move astro config validation after astro:config:setup

* feat: superRefine

* fix: test

* feat: update test

* feat: update test

* fix: test

* feat: feedback

* feat: improve logging

* feat: refactor

* fix: no spread

* feat: split schemas

* chore: changeset

* Update packages/astro/src/core/config/schemas/README.md

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

* Update packages/astro/src/integrations/hooks.ts

* Update packages/astro/src/core/config/schemas/README.md

* Update .changeset/easy-vans-laugh.md

* Update .changeset/easy-vans-laugh.md

* grammar nit in changeset

---------

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>

Co-authored-by: ematipico <602478+ematipico@users.noreply.github.com>
Co-authored-by: delucis <357379+delucis@users.noreply.github.com>
Co-authored-by: sarah11918 <5098874+sarah11918@users.noreply.github.com>
@florian-lefebvre florian-lefebvre mentioned this pull request Sep 19, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ematipico ematipico ematipico approved these changes

@sarah11918 sarah11918 sarah11918 approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

@delucis delucis Awaiting requested review from delucis

Assignees

@florian-lefebvre florian-lefebvre

Labels

docs pr pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release

Projects

None yet

Milestone

v5.6.0

Development

Successfully merging this pull request may close these issues.

4 participants

@florian-lefebvre @ematipico @delucis @sarah11918