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

Race between showNotification & getNotifications #90

Closed
jakearchibald opened this issue Jan 11, 2017 · 2 comments
Closed

Race between showNotification & getNotifications #90

jakearchibald opened this issue Jan 11, 2017 · 2 comments

Comments

@jakearchibald
Copy link
Contributor

jakearchibald commented Jan 11, 2017

registration.showNotification('test', {
  icon: 'notification-icon.png'
}).then(() => registration.getNotifications()).then(notifications => {
  // ...
});

According to the current spec, it seems likely that the notification shown by showNotification won't be in the sequence returned by getNotifications.

showNotification resolves before fetching icons & running display steps, and the notification isn't added to the list of notifications until after the display steps.

Possible solutions: showNotification should resolve later, once the notification is shown. Or, the notification should be added to the list of notifications sooner. Or, getNotifications should wait for pending notifications.

The first solution feels simplest.

@annevk
Copy link
Member

annevk commented Jan 12, 2017

That sounds reasonable. I'm not even sure why we resolve early. @beverloo?

@beverloo
Copy link
Member

beverloo commented Jan 12, 2017

We actually implement that part of the spec incorrectly then - Chrome resolves once the notification has been shown. This really helps in avoiding internal logic for keeping things around until the notification is ready, e.g. after a push event where all browsers currently have notification requirements.

Waiting LGTM.

jakearchibald added a commit to jakearchibald/notifications-1 that referenced this issue Jan 12, 2017
jakearchibald added a commit to jakearchibald/notifications-1 that referenced this issue Jan 12, 2017
jakearchibald added a commit to jakearchibald/notifications-1 that referenced this issue Jan 13, 2017
@annevk annevk closed this as completed in bc5b359 Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants