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

Allow middleware to override the filename or media type of Response #13024

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

altano
Copy link
Contributor

@altano altano commented Jan 21, 2025

Changes

Add support for standard Content-Type and Content-Disposition http headers in SSG.

This PR is an implementation of this proposal, which seems to be mostly well received.

Why

Currently, middleware in SSR can override the filename or media type of .astro templates using the standard Content-Type and Content-Disposition http headers, and the browser will interpret it correctly. SSG will completely ignore these http headers and produce bad output, inconsistent with SSR. This PR brings SSR and SSG into alignment by adding Content-Type and Content-Disposition support to SSG builds.

This is mostly helpful for middleware that wants to modify the response for .astro templates to be file-based, but is fairly generic can have other uses as well.

Example

if this is returned by middleware when requesting opengraph-image.astro:

return new Response(body, {
  headers: {
    "Content-Type": "image/png", // Would be ignored during an SSG build
    "Content-Disposition": `inline; filename="opengraph-image.png"`,
  },
});
BEFORE AFTER
SSR Response served with Content-Type: image/png no change
SSR Response served with opengraph-image.png filename no change
SSG Response served with Content-Type: text/html Response served with Content-Type: image/png
SSG Response written to opengraph-image/index.html Response written to opengraph-image.png

How this will be used

The main use case I have in mind for this change is an integration that allows people to define opengraph images using .astro templates that result in png images. This is not possible today, but would be trivial if this PR is merged. I have published a proof of concept integration that works in SSR but doesn't work in SSG, but would work everywhere with this PR.

Addressing Discusssion

Using standard http headers

There was some discussion of not using standard http headers. I think we absolutely should use these headers because:

  • it makes SSR/SSG configuration symmetrical.
  • a user might set both the custom configuration AND use the http headers (which already work in SSR today) and we'd have to decide how to handle that, with no good options that are obvious to me.

I feel strongly we should use standard http headers.

Other usecases

There was [some discussion] of other usecases (e.g. withastro/roadmap#643 (comment)) that I haven't tested but should be handled by this feature as-implemented.

Asymmetry between SSR/SSG

There was a conversation about this potentially introducing asymmetry between SSR/SSG.

I think this is a very valid concern, but, while this could be used to introduce asymmetry but it can also be used to fix asymmetry that is impossible to fix today. Example in the discussion.

Concerns

My current implementation has one issue which should either be discussed: if you dynamically use middleware to rewrite where you output the result of a request for a .astro template, the manifest/assets will not be exactly correct. For example, you might rewrite a .astro template to a png image, but the image will not be in the SSR manifest assets. The build asset URLs will reflect the correct output URL though, and I wrote a unit test that captures this.

I am unclear on how these integration hooks (which are the only way I know of to introspect the manifest during a build) are meant to be used, so I have no idea if this is a problem or not. I am unable to fix it though because we discover middleware rewriting AFTER we do the Vite build, which is where the manifest is generated.

Testing

I have added several unit tests to test the basic functionality. I have not extensively tested every edge/error case but would be happy to add more tests if this PR makes it far enough.

Docs

This is a new feature which would require documentation. I have not yet written that documentation but will if I get the provisional go-ahead.

This is a new feature that is unlikely to be incompatible with any existing astro sites. This would break backwards compatibility if someone is using Content-Disposition with .astro templates in SSG today, but since that does nothing and is totally broken, I can't imagine this is in use anywhere. So this should be documentation for a new feature.

This change does cause the existing build.format configuration option to be ignored for non-html astro templates where the filename is overridden, and I have updated the documentation to capture this. (jsdoc comment that backs the Configuration Reference documentation.

Copy link

changeset-bot bot commented Jan 21, 2025

🦋 Changeset detected

Latest commit: 52eec79

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 pkg: astro Related to the core `astro` package (scope) docs pr semver: minor Change triggers a `minor` release labels Jan 21, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@ematipico
Copy link
Member

ematipico commented Jan 21, 2025

The presence of these concerns and open-ended questions makes me think that maybe this feature should be moved into a RFC process because that's where these questions get answered.

Also, this feature seems to be middleware-centric because that's where headers can be injected, but that's wrong, and this proposal should address that.

For example, headers can be set in Astro frontmatter too. Also, what about endpoints? An endpoint can return a Response there, which can contain any header, including Content-Disposition and they work either in SSG and SSR. This RFC should have addressed that.

I don't mind the feature, I think it's really valuable, but there are a lot of unknowns that should be addressed beforehand.

@altano
Copy link
Contributor Author

altano commented Jan 21, 2025

The presence of these concerns and open-ended questions makes me think that maybe this feature should be moved into a RFC process because that's where these questions get answered.

I feel like I’m misunderstanding the process because I did open a discussion and that’s how you start the RFC process, right? That discussion has supportive comments from the core Astro team and I didn’t see any talk of stages, so I’m not sure what the next step should be. Please let me know how to progress the RFC. I can close this PR if you want.

@ematipico
Copy link
Member

ematipico commented Jan 21, 2025

Please let me know how to progress the RFC

It's literally the link I gave you in my previous message: https://github.com/withastro/roadmap/blob/main/README.md

@altano
Copy link
Contributor Author

altano commented Jan 21, 2025

I did read the link. I just don’t know how to proceed. Is my proposal implicitly in stage 1 now?

A proposal reaches this stage (aka "is accepted") during an [#rfc-meetings](RFC Meeting) with Maintainers and TSC (>= L2), following our existing RFC voting process.

Was my proposal discussed at an RFC meeting? There’s no mention of that either way in the comments. How do I get it in front of an RFC meeting such that I can get to stage 2 and create an RFC issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs pr 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