Added config option for setting a hostname to be prepended to Server Island URLs#17121
Added config option for setting a hostname to be prepended to Server Island URLs#17121ryechus wants to merge 12 commits into
Conversation
🦋 Changeset detectedLatest commit: e950d79 The changes in this PR will be included in the next version bump. This PR includes changesets to release 392 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 |
Merging this PR will improve performance by 10.84%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | Build: full server site |
1.3 s | 1.2 s | +10.84% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ryechus:mhardin/add-serverIslandHostname (e950d79) with main (164df87)1
Footnotes
matthewp
left a comment
There was a problem hiding this comment.
We have a similar option for assets here: https://docs.astro.build/en/reference/configuration-reference/#buildassetsprefix
So I think this should follow that convention.
Also, yes this does need docs.
|
Blocked on naming. I'm not sure if it should be |
This has been addressed! I also made a PR for docs. |
|
@matthewp I'm seeing a comment on the docs PR that my changes in the jsDocs should be enough if this is a config only change. Did you have something else in mind? |
ematipico
left a comment
There was a problem hiding this comment.
I doubt this feature is so small. A "server island" prefix is a concern of adapters too, but I don't see any changes or tests for them.
The assets prefix has more logic behind it.
Also, for example, docs and PR fail to document how a user can move server islands into a different CDN.
| 'astro': minor | ||
| --- | ||
|
|
||
| Added config option for setting a hostname to be prepended to Server Island URLs. This will allow users to deploy Server Island server-side code seperately from generated static output. |
There was a problem hiding this comment.
This is a minor, a new feature, so we should show more love. Here's some tips on how to do that: https://contribute.docs.astro.build/docs-for-code-changes/changesets/#new-features
Thanks for the comment. I'd like to address it, but I'm not sure I understand how this change would be a concern for adapters. Would you mind providing a little more context? |
|
Server islands are supported only if you have an adapter, without one you can't use server island, so they are an adapter concern. If you use the Node.js adapter, how would it serve the islands from the prefix? Some tests, at least, are needed |
Changes
serverIslandHostnameconfig.basesuch that there won't be duplicated slashesTesting
Tested using a local project that will make use of the feature.
Smoke tests:
astro dev-- with new config set -- observe that the config value is included at the beginning of the server island URL
-- with the new config unset -- observe that the config value is unchanged
astro build-- with new config set -- observe that generated static code includes new config value at the beginning of the server island URL
-- with the new config unset -- observe that the config value is unchanged
Ran unit tests for
packages/astroDocs
@withastro/maintainers-docs
This could affect the user's behavior if a user sets this value and isn't aware that they need to deploy the server side application to wherever this new hostname is proxying.
withastro/docs#14130