feat!: bring dev server headers to parity with production #5490
Conversation
📊 Benchmark resultsComparing with 171a14a Package size: 263 MB⬆️ 0.02% increase vs. 171a14a Legend
|
| @@ -133,7 +132,6 @@ export const initializeProxy = async ({ | |||
| [headers.Functions]: functionNames.join(','), | |||
| [headers.ForwardedHost]: `localhost:${passthroughPort}`, | |||
| [headers.Passthrough]: 'passthrough', | |||
| [headers.RequestID]: generateUUID(), | |||
There was a problem hiding this comment.
Moved to the proxy so that all requests have it.
|
@khendrikse I added and updated some tests so your review got stale. Could you give it another look, please? |
src/utils/request-id.mjs
Outdated
|
|
||
| // Transform a v4 UUID to match the format used in production — alphanumeric | ||
| // characters, all uppercase, 26 characters. | ||
| export const generateRequestID = () => generateUUID().replace(/-/g, '').toUpperCase().slice(0, 26) |
There was a problem hiding this comment.
if we wanted, we could use https://github.com/ulid/javascript to get the very same format as is being used in production, so if any of our users rely on the characteristics of ULIDs (e.g. the embedded timestamp), that'd make it easier to test in dev. I don't think anybody does that, nor that anybody should, so it's probably right to use UUID here. And it sames some NPM dead weight.
There was a problem hiding this comment.
Yeah I was torn on this myself. But if the idea is to bring parity with production as closely as possible, we might as well use the same format. Updated in d0227f7.
| @@ -21,7 +21,8 @@ export const startStaticServer = async ({ settings }) => { | |||
| }) | |||
|
|
|||
| server.addHook('onRequest', (req, reply, done) => { | |||
| reply.header('X-Powered-by', 'netlify-dev') | |||
There was a problem hiding this comment.
should we add the Server: Netlify header? or Server: Netlify Dev, as we call it in https://docs.netlify.com/cli/get-started/#get-started-with-netlify-dev?
There was a problem hiding this comment.
Unless there's a reason to differentiate them, I'd rather use the same value in both environments.
There was a problem hiding this comment.
makes sense. right now we're not setting Server anywhere, right? should we?
There was a problem hiding this comment.
We're not. Production gives you server: Netlify and CLI gives you x-powered-by: netlify-dev.
There was a problem hiding this comment.
that's what I figured. This PR is about moving the CLI closer to prod together, so let's add reply.header("Server", "Netlify")
There was a problem hiding this comment.
I'm confused. That's what we're doing here, except we're doing it for all responses and not just the ones served by the static server.
There was a problem hiding this comment.
I was confused. Didn't see that, sorry.
eduardoboucas
dismissed stale reviews from Skn0tt and khendrikse
via
d0227f7
eduardoboucas
changed the title
|
Changed to |
Summary
Some of the headers returned by the CLI's dev server are different from what we return in production. This PR attempts to bring them closer.