fix(Astro global): parity with ctx.site
#10325
Conversation
🦋 Changeset detectedLatest commit: 41d58b2 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
bot
added
pkg: astro
|
We're not supposed to modify the user's |
|
Yes, that's correct. We are currently adding base to it - this PR makes it so that we don't. |
| site: 'https://mysite.dev/blog/', | ||
| site: 'https://mysite.dev/subsite/', |
There was a problem hiding this comment.
A subpath to the site seems unnatural. It is possible it was not the intention of the orignal PR in the first place.
|
I don't know enough about how computeCurrentLocale works to tackle it myself. I can open a p1. |
Linear tickets should be fine |
| @@ -131,157 +80,3 @@ class Slots { | |||
| return outHTML; | |||
| } | |||
| } | |||
|
|
|||
| export function createResult(args: CreateResultArgs): SSRResult { | |||
There was a problem hiding this comment.
Why was this function moved out?
There was a problem hiding this comment.
So that the code for the Astro global could live alongside that of APIContext and share the implementation details.
| // TODO: look into why computeCurrentLocale() needs routeData.route to pass ctx.currentLocale tests, | ||
| // and url.pathname to pass Astro.currentLocale tests. | ||
| // A single call with `routeData.pathname ?? routeData.route` as the pathname still fails. |
There was a problem hiding this comment.
Yeah, this puzzled me, too. It requires some investigation. We definitely know that url.pathname doesn't make the cut because during the build, it contains the actual name of the file, e.g. /index.html//blog/en.html. However, in dev it contains the correct data.
| Object.defineProperty(response, 'headers', { | ||
| value: response.headers, | ||
| enumerable: true, | ||
| writable: false, | ||
| }); |
There was a problem hiding this comment.
nit: for a better UX, it's better to provide a custom set that throws an AstroError, so we can explain to the user why the operation is forbidden,
|
Great, merging after tests. |
Changes
Astroglobal alongside that ofAPIContext.baseout of the picture while generatingmanifest.site.Testing
astro-global.test.jsandssr-api-route.test.jsDocs