-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
chore: update adapters to better support astro v5 #454
Conversation
🦋 Changeset detectedLatest commit: e874db6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 49 packages
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 |
cc @florian-lefebvre any idea why |
Thinking more bout this. |
|
There will be some cascading needed here or the adapter will install/require both Astro 4 and 5. I'll release Astro When right after the official v5 tomorrow. |
@Fryuni I don't think it's related. The failing test should work without |
@florian-lefebvre you can run the
|
Not related to the error, but related to this PR |
I've tested a few things locally and I think the following is happening:
|
The image endpoint should always be injected when the buildOutput is server. It'd be great if it was only injected when needed, but it's unfortunately not really possible. |
Alright then it's possible I'm calling the hook right before those routes are injected, I'll check tomorrow |
@alexanderniebuhr I've created a preview release |
@florian-lefebvre it doesn't seem to change anything. Maybe I'm missing more here, but since the old hook works, this seems to be an issue with the new hook 🤔 |
That's weird, I'm going to look deeper now |
I updated all astro versions to the preview release so that the changes take effect for some reason. But there's one test failing now and it's unclear to me why. I think the fact that the server islands route is now part of the manifest changes how this test |
I'll check the test later, it might be an outdated assertion or it's a bug. |
@florian-lefebvre I am still new to Astro, but assuming, without this fix Image component wouldn't work in Server Islands for CloudFlare? Here are the html output of Image component in Server Islands but all images are not loaded correctly (not showing up). |
Hi @monaye I don't know actually! I think it's worth opening a new issue for what you're facing |
@alexanderniebuhr tests are now passing, can you double check the changes? I'll also do a ptal |
Just a clarification; there was a general problem with deploying Astro v.5 to Cloudflare that will be fixed with this PR? Other PRs mentioned Vue as well. |
It may not be fixed by this PR no |
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.
@florian-lefebvre I think it should be good to go. Have we adapted the adapterFeatures to fix #191 (comment) aswell?
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.
@florian-lefebvre I think it should be good to go. Have we adapted the adapterFeatures to fix #191 (comment) too?
@lassegit What general problem are you talking about? If you mean the Vue/Svelte specific issues, they won't be fixed with this PR. However Astro v5 should be deployable to Cloudflare today. |
@alexanderniebuhr I just check with the latest dependencies and the issue seems resolved. Thanks. |
@@ -476,6 +488,28 @@ export default function vercelAdapter({ | |||
}; | |||
} | |||
|
|||
function resolvedRouteToRouteData( |
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.
Do we plan to ultimately refactor this away? If so, maybe we can add a TODO here (and to the same functions copied in different adapters.)
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.
Yes utlimately we need to update the function from the underscore redirects package, that would be breaking tho. Since it's in v0, I guess it'd be fine to ship the update as a minor.
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.
I've created a linear task, I'll take care of it this week
Changes
astro:routes:resolved
@astrojs/cloudflare
is not compatible with the image service "Sharp". #191Testing
Docs