-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add support for headers config #200
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
Conversation
0e08454
to
ff08f9a
Compare
8d6189e
to
ccf12b1
Compare
"description": "TypeScript implementation of Netlify's headers engine", | ||
"type": "module", | ||
"engines": { | ||
"node": "^18.14.0 || >=20" |
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.
See also #210
b0d4959
to
b881f9f
Compare
b881f9f
to
1f877fe
Compare
1f877fe
to
4db3200
Compare
packages/dev/package.json
Outdated
@@ -57,6 +57,7 @@ | |||
"@netlify/config": "^23.0.4", | |||
"@netlify/dev-utils": "2.2.0", | |||
"@netlify/functions": "3.1.9", | |||
"@netlify/headers": "^0.0.0", |
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 think we want to pin an exact version here.
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'll make that change for internal consistency, but what are we solving for here and how do we plan to enforce it?
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.
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.
When referencing packages from the monorepo, we always know the exact version we want to use. We're not enforcing that anywhere.
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.
Bleh, I wish npm workspaces had better functionality here. With pnpm or yarn we would just use workspace:*
. 🤷🏼
const matchingHeaderRules = headersForPath(headerRules, new URL(request.url).pathname) | ||
|
||
for (const [key, value] of Object.entries(matchingHeaderRules)) { | ||
response.headers.set(key, value) |
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.
This won't work for any responses with immutable headers. For example:
> Response.redirect("https://example.com").headers.set("X-Foo", "bar")
Uncaught TypeError: immutable
at _Headers.set (node:internal/deps/undici/undici:8609:17)
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.
Interesting. Wouldn't this never apply here though? We only ever apply header rules to static files.
Are you suggesting we implement the guard as a failsafe?
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.
Hmm, good point. It wouldn't hurt to have it as a failsafe, but not critical.
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.
LGTM! The only thing missing is adding the new package to the release flow:
# Publishing packages in topological order, as defined in `package.json`. |
Summary
This adds support for static response headers configuration: https://docs.netlify.com/routing/headers/.
This is largely ported from analogous code in Netlify CLI. As noted here (private link), this does avoid porting over a bug I noticed.