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

Add server.headers option #5564

Merged
merged 1 commit into from
Dec 14, 2022
Merged

Conversation

riywo
Copy link
Contributor

@riywo riywo commented Dec 9, 2022

With this new server.headers option, the users can specify custom headers for astro dev and astro preview servers.

This is useful when they want to build a website requiring specific response headers such as Cross-Origin-Opener-Policy.

Changes

Adding a new server.headers option to allow the users to specify HTTP response headers for both astro dev and astro preview.

Testing

Added new unit tests for both astro dev and astro preview.

Docs

This document should be updated: https://docs.astro.build/en/reference/configuration-reference/#server-options

@changeset-bot
Copy link

changeset-bot bot commented Dec 9, 2022

🦋 Changeset detected

Latest commit: 121ed37

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 Dec 9, 2022
@riywo riywo force-pushed the feature/server.headers branch from 22c34bc to a34a19e Compare December 9, 2022 02:54
Comment on lines +147 to +151
// Set user specified headers to response object.
for (const [name, value] of Object.entries(config.server.headers ?? {})) {
if (value) res.setHeader(name, value);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why but passing server.headers to vite config didn't work. Hence, writing the headers here directly.

@riywo
Copy link
Contributor Author

riywo commented Dec 9, 2022

Can anyone rerun the failed job? It failed at post steps, so it actually succeeded. https://github.com/withastro/astro/actions/runs/3654029967/jobs/6174109857

@riywo
Copy link
Contributor Author

riywo commented Dec 10, 2022

Thank you for kicking a retry. Now all checks have passed.

It would be great if someone can review and merge this change. I'd like to build a website using SharedArrayBuffer which requires some HTTP headers and this change will help such use cases. Thanks!

@riywo
Copy link
Contributor Author

riywo commented Dec 10, 2022

Hmm, checking this patch with a real environment but it doesn't seem working. Will update.

With this new `server.headers` option, the users can specify
custom headers for `astro dev` and `astro preview` servers.

This is useful when they want to build a website requiring
specific response headers such as `Cross-Origin-Opener-Policy`.
@riywo riywo force-pushed the feature/server.headers branch from a34a19e to 121ed37 Compare December 10, 2022 03:07
@riywo
Copy link
Contributor Author

riywo commented Dec 10, 2022

Done. This patch was needed when invoked from CLI actually. https://github.com/withastro/astro/compare/a34a19e8c92b5fcb92fe2651369e68465f090405..121ed37bdde07213628d9347405581d0c5a187c8

Not sure we should add such unit tests (or E2E tests?), though.

@matthewp
Copy link
Contributor

I think this is good since this is done manually. In production you'll have to make sure your server is configured to do the same, but I think given this is an opt-in approach that becomes obvious.

@matthewp matthewp added the semver: minor Change triggers a `minor` release label Dec 12, 2022
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Blocking because this is a minor change. It will go out with the next minor release (likely on Thursday)

@matthewp matthewp dismissed their stale review December 14, 2022 15:24

Today is merge day

@matthewp matthewp merged commit dced4a8 into withastro:main Dec 14, 2022
@astrobot-houston astrobot-houston mentioned this pull request Dec 14, 2022
@riywo riywo deleted the feature/server.headers branch December 14, 2022 15:40
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) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants