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

Run astro:config:done hook before generating route manifest #12936

Closed
wants to merge 1 commit into from

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jan 8, 2025

Changes

fix #12803

Testing

n/a. it's a bit hard to add test for this one

Docs

n/a. bug fix.

Copy link

changeset-bot bot commented Jan 8, 2025

🦋 Changeset detected

Latest commit: eca2174

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

settings.buildOutput = getPrerenderDefault(config) ? 'static' : 'server';
// NOTE: `buildOutput` may already be set by `runHookConfigDone` if the adapter only
// support server build output
settings.buildOutput ??= getPrerenderDefault(config) ? 'static' : 'server';
Copy link
Member

Choose a reason for hiding this comment

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

So the reordering of hooks is for this to work right? Just to make sure I understand, we don't want to infer the build output if the adapter locks it. Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. This code will only work as a fallback

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jan 8, 2025
@florian-lefebvre
Copy link
Member

This requires changes to the integration reference:

  • the ordering of hooks in code snippets, and even in the document
  • update hooks section that tlak about previous/next hook

@bluwy
Copy link
Member Author

bluwy commented Jan 8, 2025

Yeah I suppose we have to update it if this PR works. Would technically be kinda breaking, though otherwise I don't see an easy way else to fix this.

@florian-lefebvre
Copy link
Member

florian-lefebvre commented Jan 8, 2025

Yes technically breaking, I have some integrations relying on the order. That being said, I think the hook is recent enough so it's not used a lot yet, let's do it if it's the only way. Most important thing is to check it doesn't break official adapters

Copy link

codspeed-hq bot commented Jan 8, 2025

CodSpeed Performance Report

Merging #12936 will not alter performance

Comparing route-manifest-adapter (eca2174) with main (fd12a26)

Summary

✅ 6 untouched benchmarks

@bluwy
Copy link
Member Author

bluwy commented Jan 14, 2025

I think an alternative to this is to directly set the output to server in the node adapter. That way we don't have to make this breaking change. But the behaviour if an adapter doesn't set the output and causes these kind of problems is still something we should look into though.

EDIT: No this isn't quite enough

@bluwy
Copy link
Member Author

bluwy commented Jan 14, 2025

Closing as I've got a different fix.

@bluwy bluwy closed this Jan 14, 2025
@trueberryless
Copy link

Is this the recommended fix now: #12982?

@bluwy
Copy link
Member Author

bluwy commented Jan 16, 2025

Yeah, I was testing it a bit more and I think that's the proper solution for now.

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.

Server islands respond with 404 when deployed to vercel
3 participants