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 (un)register to (un)subscribe #98

Closed
johnmellor opened this issue Dec 15, 2014 · 7 comments
Closed

Rename (un)register to (un)subscribe #98

johnmellor opened this issue Dec 15, 2014 · 7 comments

Comments

@johnmellor
Copy link
Contributor

@costinm mentioned that it would make more sense to use the term (un)subscribe instead of (un)register, since the app is not registering for an account, instead it's subscribing to a source of messages (this also matches better with terminology like Pub/Sub).

@martinthomson
Copy link
Member

I've started using the terms subscribe/unsubscribe after discussions at the IETF around terminology. This is better, in my opinion, since it fits the messaging model better. Of course, none of the other SW APIs do that. What do you want consistency with? Unless of course, we can change the naming used elsewhere...

@johnmellor
Copy link
Contributor Author

The other SW APIs are less related to messaging though, so perhaps it's ok to be named differently? i.e. perhaps the following is actually fine?:

navigator.serviceWorker.register(...);
navigator.serviceWorker.ready.then(registration -> {
    registration.pushManager.subscribe().then(...);
});

I suppose it does conflict a little with the serviceWorkerRegistration.syncManager.register API currently suggested for Background Sync in WICG/background-sync/issues/20, though there's still time to change that.

@jkarlin
Copy link

jkarlin commented Dec 16, 2014

I don't think that subscribe/unsubscribe makes sense for Background Sync.

@mvano
Copy link
Contributor

mvano commented Jan 6, 2015

I don't feel very strongly about subscribe and subscription vs register and registration. The churn in spec, implementations, and demos seems hardly worth it.

Subscribe may seem nice when phrased as "subscribing to a source of push messages" but it also seems a little weird as you don't even specify the source that you are subscribing to. You're acquiring an endpoint to which any number of apps can send messages, and it has an id. It seems like "register" and "registration" cause very little confusion as a name for these concepts.

In favor of sticking with register: it retains consistency with the Service Worker, Sync, and Geofencing APIs.

CC @jakearchibald for the Service Worker perspective

@beverloo
Copy link
Member

beverloo commented Jan 6, 2015

Churn in the specification and/or demos for something that hasn't shipped anywhere yet really isn't an argument. There are no backwards compatibility concerns here.

I think that describing this as "subscribing to a push channel" makes sense -- the user is essentially subscribed to receiving a flow of messages from the push service. I'd be in favor of doing the rename.

@martinthomson
Copy link
Member

If this isn't renamed, we're going to have a problem with the protocol.

@mvano
Copy link
Contributor

mvano commented Jan 7, 2015

Looking at the latest web push protocol draft [1] I see now that it uses both terms. I agree that subscribe/subscription is the better term to use as it maps to the concept of the same name in the protocol.

I'll take a look at making this change in the Push API spec.

[1] https://martinthomson.github.io/drafts/draft-thomson-webpush-http2.html

@mvano mvano closed this as completed Jan 8, 2015
momchil-velikov pushed a commit to eth-sri/BlinkER that referenced this issue Jan 14, 2015
This is in response to this spec issue:
w3c/push-api#98

Depends on https://codereview.chromium.org/827083004/

BUG=446883

Review URL: https://codereview.chromium.org/841333002

git-svn-id: svn://svn.chromium.org/blink/trunk@188173 bbb929c8-8fbe-4397-9dbb-9b2b20218538
ltilve pushed a commit to ltilve/chromium that referenced this issue May 22, 2015
BUG=446883

This is in response to this spec issue:
w3c/push-api#98

Review URL: https://codereview.chromium.org/1134733006

Cr-Commit-Position: refs/heads/master@{#331062}
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

No branches or pull requests

5 participants