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

Rename response headers to match implementation. #772

Merged
merged 6 commits into from
Sep 12, 2023

Conversation

qingxinwu
Copy link
Collaborator

@qingxinwu qingxinwu commented Aug 29, 2023

Rename response headers in both explainer and spec to use new headers in implementation. The request headers of "X-..." has been renamed (although the old headers are still supported to be backward compatible). See the bug that tracks the work.

  • X-Allow-FLEDGE -> Ad-Auction-Allowed
  • X-FLEDGE-Auction-Only -> Ad-Auction-Only

Not renaming X-fledge-bidding-signals-format-version since this header is going away soon.


Preview | Diff

@qingxinwu qingxinwu requested a review from JensenPaul August 29, 2023 17:20
@qingxinwu
Copy link
Collaborator Author

qingxinwu commented Aug 29, 2023

@JensenPaul Mind reviewing since the change? I can ask Caleb to review it instead if you'd like to.

@qingxinwu
Copy link
Collaborator Author

@domfarolino mind taking a look at the small change to the spec in this PR? Thanks!

@MattMenke2
Copy link
Contributor

I'd rather we not rename X-fledge-bidding-signals-format-version. The goal is to remove it, and we're only seeing like 1% of requests using version 1.

@qingxinwu
Copy link
Collaborator Author

qingxinwu commented Aug 30, 2023

I'd rather we not rename X-fledge-bidding-signals-format-version. The goal is to remove it, and we're only seeing like 1% of requests using version 1.

Done.

@domfarolino
Copy link
Collaborator

Rename request headers to match implementation in both explainer and spec.

Do you mean "implementation and both explainer and spec"? If we were just renaming these to match the implementation, I'd expect renaming X-fledge-bidding-signals-format-version to be uncontroversial, but #772 (comment) says otherwise. So are we renaming these to match what the implementation already supports on stable, or are we skipping ahead past the implementation a lot? Or just a little?

@MattMenke2
Copy link
Contributor

Rename request headers to match implementation in both explainer and spec.

Do you mean "implementation and both explainer and spec"? If we were just renaming these to match the implementation, I'd expect renaming X-fledge-bidding-signals-format-version to be uncontroversial, but #772 (comment) says otherwise. So are we renaming these to match what the implementation already supports on stable, or are we skipping ahead past the implementation a lot? Or just a little?

I don't think we want to ask people to update their code for something we hope to remove in maybe a month or two - we're only seeing this in 0.5% of requests, down from 1.5% a month ago, and print out a devtools warning on the old version. The new version allows new keys to be added to the top-level dictionary if we need new types of data, so hopefully it's not something we'll need in the future - if we need new versions, we could even just include them in the JSON response body, something we couldn't do before, since every key had a meaning.

I think we actually do support all the new headers, I just think in this case, we should not be advertising it, or have made the change in the first place. I wasn't a party to the code change here (was out on medical leave at the time), or I would have pushed back on it.

@qingxinwu
Copy link
Collaborator Author

qingxinwu commented Aug 31, 2023

Rename request headers to match implementation in both explainer and spec.

Do you mean "implementation and both explainer and spec"? If we were just renaming these to match the implementation, I'd expect renaming X-fledge-bidding-signals-format-version to be uncontroversial, but #772 (comment) says otherwise. So are we renaming these to match what the implementation already supports on stable, or are we skipping ahead past the implementation a lot? Or just a little?

I don't think we want to ask people to update their code for something we hope to remove in maybe a month or two - we're only seeing this in 0.5% of requests, down from 1.5% a month ago, and print out a devtools warning on the old version. The new version allows new keys to be added to the top-level dictionary if we need new types of data, so hopefully it's not something we'll need in the future - if we need new versions, we could even just include them in the JSON response body, something we couldn't do before, since every key had a meaning.

I think we actually do support all the new headers, I just think in this case, we should not be advertising it, or have made the change in the first place. I wasn't a party to the code change here (was out on medical leave at the time), or I would have pushed back on it.

Yes, for all the headers here, our implemention now supports both the new and old headers (to be backward compatible). We expect people to switch to new headers, and stop supporting old headers when no longer needed.
For the format version header, I agree with Matt that we don't advertise the new header (and better not have renamed it in the implementation), since we expect to remove the header soon and better let people continue using the old header.

@domfarolino
Copy link
Collaborator

Yes, for all the headers here, our implemention now supports both the new and old headers (to be backward compatible). We expect people to switch to new headers, and stop supporting old headers when no longer needed.

But will even the new headers be removed in a month or two, as Matt suggests? Or are they here to say?

@qingxinwu
Copy link
Collaborator Author

qingxinwu commented Aug 31, 2023

Yes, for all the headers here, our implemention now supports both the new and old headers (to be backward compatible). We expect people to switch to new headers, and stop supporting old headers when no longer needed.

But will even the new headers be removed in a month or two, as Matt suggests? Or are they here to say?

For the format version one, yes, the new header will be removed as well when the old one is removed.

@domfarolino
Copy link
Collaborator

Ah OK, and the other ones will continue to exist forever?

@MattMenke2
Copy link
Contributor

No plans to remove any of the others, but the heat death of the universe is a pretty long ways off, so reluctant to commit to something on that timeframe.

@qingxinwu qingxinwu changed the title Rename request headers to match implementation. Rename response headers to match implementation. Sep 1, 2023
spec.bs Show resolved Hide resolved
@JensenPaul JensenPaul merged commit f6a40a8 into WICG:main Sep 12, 2023
1 check passed
github-actions bot added a commit that referenced this pull request Sep 12, 2023
SHA: f6a40a8
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@qingxinwu qingxinwu deleted the header branch September 12, 2023 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants