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

Change name from targetClientId to replacesClientId #1333

Merged
merged 1 commit into from
Aug 26, 2018

Conversation

jungkees
Copy link
Collaborator

@jungkees jungkees commented Jul 1, 2018

We had decided (and I forgot) to change the name of FetchEvent.targetClientId to
.replacesClientId to clarify the meaning that this client is a to be replaced
client: #1091 (comment).

Related issue: #1245.

Fetch PR: whatwg/fetch#774.
HTML PR: whatwg/html#3788.


Preview | Diff

We had decided (and I forgot) to change the name of FetchEvent.targetClientId to
.replacesClientId to clarify the meaning that this client is a to be replaced
client: #1091 (comment).

Related issue: #1245.
@jungkees jungkees requested a review from jakearchibald July 1, 2018 22:58
@jungkees
Copy link
Collaborator Author

jungkees commented Jul 1, 2018

I'll make PRs for Fetch and HTML changes.

jungkees added a commit to jungkees/fetch that referenced this pull request Jul 1, 2018
We had decided to change the name of FetchEvent.targetClientId to
.replacesClientId to clarify the meaning that this client is a to be replaced
client: w3c/ServiceWorker#1091 (comment).
Accordingly, this changes the name of the request's target client id item to
replaces client id.

Related issue: w3c/ServiceWorker#1245.

SW PR: w3c/ServiceWorker#1333.
jungkees added a commit to jungkees/html that referenced this pull request Jul 1, 2018
We had decided to change the name of FetchEvent.targetClientId to
.replacesClientId to clarify the meaning that this client is a to be replaced
client: w3c/ServiceWorker#1091 (comment).
Accordingly, this changes the reference to the request's target client id to
request's replaces client id.

Related issue: w3c/ServiceWorker#1245.

SW PR: w3c/ServiceWorker#1333.
Fetch PR: whatwg/fetch#774.
annevk pushed a commit to whatwg/fetch that referenced this pull request Aug 24, 2018
We had decided to change the name of FetchEvent's targetClientId to replacesClientId to clarify the meaning that this client is a to be replaced client: w3c/ServiceWorker#1091 (comment). Accordingly, this changes the name of request's target client id to replaces client id.

See also: 

* w3c/ServiceWorker#1245
* w3c/ServiceWorker#1333
* whatwg/html#3788
annevk pushed a commit to whatwg/html that referenced this pull request Aug 24, 2018
We had decided to change the name of FetchEvent's targetClientId to replacesClientId to clarify the meaning that this client is a to be replaced client: w3c/ServiceWorker#1091 (comment). Accordingly, this changes the reference to the request's target client id to request's replaces client id.

See also:

* w3c/ServiceWorker#1245
* w3c/ServiceWorker#1333
* whatwg/fetch#774
@annevk
Copy link
Member

annevk commented Aug 24, 2018

This can be reviewed and landed (if it passes review). Fetch and HTML are updated.

@annevk
Copy link
Member

annevk commented Aug 24, 2018

(It might take a day for Bikeshed to have the new terms indexed though.)

@jungkees
Copy link
Collaborator Author

Thanks @annevk. I'll merge this tomorrow.

@jungkees jungkees merged commit d2391d4 into master Aug 26, 2018
@jungkees jungkees deleted the change-to-replacesclientid branch August 26, 2018 19:02
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
We had decided to change the name of FetchEvent's targetClientId to replacesClientId to clarify the meaning that this client is a to be replaced client: w3c/ServiceWorker#1091 (comment). Accordingly, this changes the reference to the request's target client id to request's replaces client id.

See also:

* w3c/ServiceWorker#1245
* w3c/ServiceWorker#1333
* whatwg/fetch#774
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
We had decided to change the name of FetchEvent's targetClientId to replacesClientId to clarify the meaning that this client is a to be replaced client: w3c/ServiceWorker#1091 (comment). Accordingly, this changes the reference to the request's target client id to request's replaces client id.

See also:

* w3c/ServiceWorker#1245
* w3c/ServiceWorker#1333
* whatwg/fetch#774
foolip added a commit to foolip/browser-compat-data that referenced this pull request Jun 2, 2021
@foolip
Copy link
Member

foolip commented Jun 2, 2021

I'm coming across this issue via BCD: mdn/browser-compat-data#10717

It looks like this property shipped in Safari in April 2018, and then was renamed after that. I can't find any WebKit bug filed. This looks like an unfortunate mishap, but does someone who was involved with this know more, and can someone file that WebKit bug? Historical tests in WPT would be great too.

foolip added a commit to mdn/browser-compat-data that referenced this pull request Jun 2, 2021
@jakearchibald
Copy link
Contributor

Yeah this isn't great. If memory serves, we were under the impression that no one had shipped this. I guess there was some sort of miscommunication.

I'll create the tests and file the WebKit issue. Thanks for pointing this out.

@foolip
Copy link
Member

foolip commented Jun 4, 2021

Thanks @jakearchibald!

I think that this sort of thing could be avoided with a faster and more reliable flow of support data from specs and releases into BCD. If there had already been a green row in https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent#browser_compatibility already, something like this might have occurred:

  • Nobody checked MDN, so the change was made
  • The spec link in the BCD entry disappeared from webref so this was flagged
  • Someone reviewing bad spec links in BCD finds their way back here within a week or two and points it out
  • The relevant implementers are looped in, maybe the change is reverted or maybe it sticks, but something happens.

@jakearchibald
Copy link
Contributor

foolip added a commit to foolip/browser-compat-data that referenced this pull request Jun 4, 2021
There was an unusual amount wrong here...

The current state of things in source:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/service_worker/fetch_event.idl;drc=fc05b522c6d4a85354c9d6f39b76c3d7f8448b68
https://github.com/mozilla/gecko-dev/blob/a4c86edb340aeee2b91a2ab617c66cb4ea7b7b66/dom/webidl/FetchEvent.webidl
https://github.com/WebKit/WebKit/blob/af262cabe3e5b067a177fb4203a7112f14028092/Source/WebCore/workers/service/FetchEvent.idl

The client attribute is removed as irrelevant as it actually never
shipped in Chrome, it was landed and reverted in 2015 and didn't reach
stable, and was removed in Gecko in 2015 before FetchEvent shipped:
https://chromium.googlesource.com/chromium/src/+/4e79b14a361c3960c11a2139df74a315b915c740
https://chromium.googlesource.com/chromium/src/+/ade5cc571923215463317b3212f7a50683f79038
mozilla/gecko-dev@7029e80

The navigationPreload attribute was the original name for
preloadResponse, but was renamed in Chromium before it reached stable:
https://chromium.googlesource.com/chromium/src/+/111601216214188fc2ace390da5b2af0095564fd
https://chromium.googlesource.com/chromium/src/+/95d26e23d47ff912eefe1d1caa7dc67552b3ffae

The preloadResponse attribute isn't in Gecko/WebKit source, see above.

The replacesClientId attribute turns out to not be in the source of any
engine (see above) and test shows it actually wasn't in Edge 18 either:
https://mdn-bcd-collector.appspot.com/tests/api/FetchEvent/replacesClientId

As a minor error, the resultingClientId attribute also wasn't in Edge 18:
https://mdn-bcd-collector.appspot.com/tests/api/FetchEvent/resultingClientId

Finally, the targetClientId was shipped in Safari, but in the spec it
was renamed to replacesClientId (see above) which hasn't shipped in any
browser: w3c/ServiceWorker#1333

Nevertheless, treat Safari's targetClientId as an alternative name for
replacesClientId to combine the entries.
@foolip
Copy link
Member

foolip commented Jun 4, 2021

If my research in mdn/browser-compat-data#10774 is correct, no browser has actually shipped replacesClientId yet. Given that, is it worth considering just renaming it back to targetClientId?

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