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

Expose pushManager on Navigator #368

Closed
wants to merge 2 commits into from
Closed

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Dec 14, 2023

Part of #360

This PR introduces:

  • navigator.pushManager
  • makes changes to the various method/algorithms of pushManager to distinguish between service worker scope and running in a regular window.

Discussion (or stuff I'm not super happy with):

  • Using the "environment settings object" is probably not the best here, because Workers are also an environment (or have an environment settings object).

The current spec is currently hand-wavy about how push registrations are persistently associated with a service worker (which is ok), but not it gets a little bit messier because we need to distinguish between the Window object associated registration and the service worker ones.

Anyone got any suggestions as to what I could use to hand the registration off instead?

The following tasks have been completed:

  • Modified Web platform tests (link to pull request)

Implementation commitment:


Preview | Diff

@marcoscaceres marcoscaceres changed the title Navigator.push manager Expose pushManager on Navigator Dec 14, 2023
@marcoscaceres
Copy link
Member Author

marcoscaceres commented Dec 14, 2023

Actually, thinking about this... if an (main thread) origin (i.e., "a site") has a persistent environment settings object, and service worker has its own independently persistent environment-settings object, then we could say that every environment has an associated push subscription (which can be null).

This could work because:

  • a service worker can has at most 1 push subscription
  • a window would have at most 1 push subscription.

Tying subscription to the environment then that could answer how push subscriptions are associated with execution contexts.

@martinthomson
Copy link
Member

Thanks for writing this up Marcos. I'm going to probably step aside on this one and defer to @saschanaz. Both of us are about to go on some leave, so I hope you don't mind if this takes a little while.

@marcoscaceres
Copy link
Member Author

No problem at all. I'll put up a few PRs over xmas and try to keep them small.

I talked to Anne about using "environment", but he suggested they are too volatile... so, I'm going to create some kind of persistent "push subscriptions store" instead. That will be just simple store that handles the association between push subscriptions, service workers, and the origin.

@saschanaz
Copy link
Member

Some early thought:

I talked to Anne about using "environment", but he suggested they are too volatile... so, I'm going to create some kind of persistent "push subscriptions store" instead. That will be just simple store that handles the association between push subscriptions, service workers, and the origin.

Perhaps we can associate push manager to storage bottle as Web Locks does for lock managers?: https://w3c.github.io/web-locks/#lock-managers

</li>
<li>Let |sw| be |registration|'s [=service worker registration/active worker=].
<li>If |sw| is null and |global| is a {{ServiceWorkerRegistration}}, [=queue a global
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ServiceWorkerGlobalScope?

foolip added a commit to mdn/browser-compat-data that referenced this pull request Apr 19, 2024
PushManager is only supported in workers, and this data should be identical to the parent feature. It's not for Safari, and that's wrong, so just remove the entry.

Note that it might be available in the window context in the future:
w3c/push-api#368
queengooborg pushed a commit to mdn/browser-compat-data that referenced this pull request Apr 28, 2024
PushManager is only supported in workers, and this data should be identical to the parent feature. It's not for Safari, and that's wrong, so just remove the entry.

Note that it might be available in the window context in the future:
w3c/push-api#368
@annevk
Copy link
Member

annevk commented Jul 17, 2024

I was looking at tackling this but ran into #384. Because #360 actually describes something that I think we can use. It suggests that a "window push subscription" is 1:1 with a push subscription for a service worker registration whose scope is / (on the same origin).

I think we should just create a service worker registration for that scope when none exists and bypass the active worker check (which seems to be there mainly for consistency).

An issue that we run into, but the existing specification also runs into, is this line in the Service Workers specification:

A service worker registration of an identical scope url when one already exists in the user agent causes the existing service worker registration to be replaced.

But it seems to me it could just as well say that it's being updated in place (or that some state, such as the push subscription, is carried over).

This setup has these benefits:

  1. Clarifies the lifetime by tying it to service worker registration lifetime. No need to integrate with Storage (though Service Workers needs to of course).
  2. Creates interoperability with existing push subscriptions on scope /.
  3. It is very simple. No need for more complicated ownership structures or anything. Only fairly modest changes to the PushManager class.

Thoughts?

@annevk
Copy link
Member

annevk commented Jul 17, 2024

@jakearchibald I know it's been a while, but maybe you could have a look at #368 (comment) given your experience with service workers. I'd appreciate it!

@mkruisselbrink
Copy link

An issue that we run into, but the existing specification also runs into, is this line in the Service Workers specification:

A service worker registration of an identical scope url when one already exists in the user agent causes the existing service worker registration to be replaced.

But it seems to me it could just as well say that it's being updated in place (or that some state, such as the push subscription, is carried over).

In practice I don't think that sentence in the service worker spec means much. The actual algorithms around registering service workers (i.e. starting with https://w3c.github.io/ServiceWorker/#start-register-algorithm) already update an existing registration in place rather than creating a new one if the scope url matches.

If I understand it correctly, I believe you're proposing introducing a service worker registration that doesn't have any service workers associated with it. That does seem a bit odd to me, although I don't think anything would necessarily break by changing that assumption... I think there might be some subtlety about what should happen when a service worker is later registered at that same scope but installation fails (should that unregister the "/" registration as currently spec'ed, or would we want to go back to having a registration without a service worker?).

@asutherland
Copy link

Having a Service Worker Registration without an active ServiceWorker feels like it discards a fairly fundamental invariant that may currently be reflected quite deeply into existing browser implementations. (It would not be a minor thing to relax this invariant in Gecko.)

Can push subscription storage just be a map from (WindowClient or ServiceWorkerRegistration) to (PushSubscription)?

And then we can sprinkle some "Eligible PushSubscription" dust on top for lifetime management of the contents of the map to deal with the fact that browsers have complex session management things going on, especially on resource-constrained devices and where the ServiceWorkers spec has not concretely addressed w3c/ServiceWorker#626. This allows browsers latitude in retaining push subscriptions for WindowClients that aren't currently loaded.

Also, this avoids complications with magic treatment of the scope "/" potentially interacting weirdly with the proposal at w3c/ServiceWorker#1512.

@annevk
Copy link
Member

annevk commented Aug 12, 2024

Thanks both for weighing in!

  1. I think the overloading is nice as it allows for reusing an existing subscription. And that ends up working in both directions. And of course you don't have to implement it as a dummy "service worker registration", that would be more of a specification device.
  2. I don't think a window client is a viable key as we wouldn't want two https://example.com/ windows to end up with separate subscriptions. In fact, https://example.com/ and https://example.com/bar should share a subscription too. (A ServiceWorkerRegistration also doesn't seem like it would be persistent enough, but maybe a "service worker registration" is.)
  3. I see your point on keying of service worker registrations potentially changing, but that also would have to be opt-in and therefore it doesn't really clash with this scheme and the default would still end up being straightforward and sharing a subscription.

@annevk
Copy link
Member

annevk commented Aug 26, 2024

The more I think about this the more I'm inclined to stick with a service worker registration as specification device as it just answers so many questions that would otherwise have to be answered in some other way for this rather small scenario.

@asutherland
Copy link

asutherland commented Aug 29, 2024

  1. I think the overloading is nice as it allows for reusing an existing subscription. And that ends up working in both directions. And of course you don't have to implement it as a dummy "service worker registration", that would be more of a specification device.

I'm confused by this. Are you suggesting that if a window registers a push subscription and the origin did not previously have a service worker registration for the scope "/" then a browser would be equally compliant if getRegistrations() returned or did not return a new registration with scope "/"?

@annevk
Copy link
Member

annevk commented Aug 29, 2024

Thanks. I need to think about the best approach some more. I forgot how web-exposed registrations are. It seems annoying to have to almost duplicate them, but maybe that's the best we can do.

@asutherland
Copy link

Maybe:

  • the push subscriptions could be defined as operating in terms of a service worker scope
  • the scope "/" can be treated specially, with it being allowed for there to be a push subscription for scope "/" without there actually being a registration. And it would be mandated that we would not actually create a registration for "/", so getRegistrations() after a window push subscription would not change anything.
  • the push subscription algorithms can use the scope to job queue map and maybe even the job coalescing system for registering/unregistering in order to provide sequencing sanity.

@annevk
Copy link
Member

annevk commented Nov 18, 2024

@asutherland I just wanted to belatedly thank you for your suggestion there. I'm pretty close to being able to provide a new take on this PR that implements that. There might still be some hand-waving, but I think it improves upon the status quo.

I will create it as a new PR as the approach changed quite a bit, but will cross-link this one.

annevk added a commit that referenced this pull request Nov 19, 2024
This is part of the Declarative Web Push initiative (see #360) and mainly makes sense when that is supported, although could be independently supported in theory.

This makes window.pushManager work by making push subscriptions tied to a scope rather than a service worker registration. Most often push subscriptions remain 1:1 with service worker registrations, but the scope whose serialized path is "/" is treated specially from now on and can exist independently.

This obsoletes #368.
@annevk annevk mentioned this pull request Nov 19, 2024
4 tasks
@annevk
Copy link
Member

annevk commented Nov 19, 2024

Closing this in favor of #393. Review appreciated!

@annevk annevk closed this Nov 19, 2024
@annevk annevk deleted the navigator.pushManager branch November 19, 2024 12:26
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.

6 participants