-
Notifications
You must be signed in to change notification settings - Fork 323
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
Powered by update #2516
Powered by update #2516
Conversation
We detected some changes in |
@@ -136,7 +136,7 @@ export const renderHydrogen = (App: any) => { | |||
|
|||
if (hydrogenConfig.poweredByHeader ?? true) { | |||
// If undefined in the config, then always show the header | |||
response.headers.set('powered-by', 'Shopify-Hydrogen'); | |||
response.headers.set('powered-by', 'Shopify, Hydrogen'); |
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.
We only have hydrogen
in v2, should we keep it that way here too?
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.
@blittle -- we should have Shopify, Hydrogen in both -- and then up to Oxygen to simply append 'Oxygen'
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.
@benjaminsehl nope, Oxygen specifically asked for it to be Hydrogen
, and they append the other two.
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.
@blittle -- OK. But off Oxygen then it will only be Hydrogen?
This ___________, so I am just trying to follow what his original request was that JS linked to (which didn't even mention Oxygen).
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.
I thought I was explicit about this when they implemented it, I thought it was weird for us to only do Hydrogen, but the consensus was we just do "Hydrogen". See their implementation here: https://github.com/Shopify/oxygen-workers/pull/853/files#diff-bbe56b6f30db247b956e162af183823b1e5c916d0b9518648fd5ee73a98b3f49R131
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.
So if we do both here, we need to update H2 as well, because it only sets powered-by: Hydrogen
Updates
powered-by
header to use current syntax