Skip to content

Commit

Permalink
refactor(service-worker): Refactorings to the service worker and SwPush
Browse files Browse the repository at this point in the history
- Rename messagesClicked to notificationClicks
- Make JSDoc for notificationClicks clearer
- Add missing property in notification options
- Use existing array of properties in handleClick method
- Use intersection type for notificationClicks instead of new interface
  • Loading branch information
joostme committed Oct 1, 2018
1 parent eef9672 commit 9db790a
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 27 deletions.
16 changes: 9 additions & 7 deletions packages/service-worker/src/push.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,16 @@ export class SwPush {
* interacted with.
* If no action was used the action property will be an empty string `''`.
*
* Note that the `notification` property is __not__ a
* [Notification](https://developer.mozilla.org/en-US/docs/Web/API/Notification) but rather a
* Note that the `notification` property is **not** a
* [Notification](https://developer.mozilla.org/en-US/docs/Web/API/Notification) object but rather
* a
* [NotificationOptions](https://notifications.spec.whatwg.org/#dictdef-notificationoptions)
* object that also includes the notification `title`.
* object that also includes the `title` of the
* [Notification](https://developer.mozilla.org/en-US/docs/Web/API/Notification) object.
*/
readonly messagesClicked: Observable < {
readonly notificationClicks: Observable < {
action: string;
notification: NotificationObject
notification: NotificationObject&{ title: string }
}
> ;
readonly subscription: Observable<PushSubscription|null>;
Expand All @@ -46,12 +48,12 @@ export class SwPush {
constructor(private sw: NgswCommChannel) {
if (!sw.isEnabled) {
this.messages = NEVER;
this.messagesClicked = NEVER;
this.notificationClicks = NEVER;
this.subscription = NEVER;
return;
}
this.messages = this.sw.eventsOfType('PUSH').pipe(map((message: any) => message.data));
this.messagesClicked =
this.notificationClicks =
this.sw.eventsOfType('NOTIFICATION_CLICK').pipe(map((message: any) => message.data));

this.pushManager = this.sw.registration.pipe(
Expand Down
6 changes: 3 additions & 3 deletions packages/service-worker/test/comm_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -304,13 +304,13 @@ import {async_fit, async_it} from './async';
});
});

describe('messagesClicked', () => {
describe('notificationClicks', () => {
it('receives notification clicked messages', () => {
const sendMessage = (type: string, action: string) =>
mock.sendMessage({type, data: {action}});

const receivedMessages: string[] = [];
push.messagesClicked.subscribe(
push.notificationClicks.subscribe(
(msg: {action: string}) => receivedMessages.push(msg.action));

sendMessage('NOTIFICATION_CLICK', 'this was a click');
Expand Down Expand Up @@ -388,7 +388,7 @@ import {async_fit, async_it} from './async';

it('does not crash on subscription to observables', () => {
push.messages.toPromise().catch(err => fail(err));
push.messagesClicked.toPromise().catch(err => fail(err));
push.notificationClicks.toPromise().catch(err => fail(err));
push.subscription.toPromise().catch(err => fail(err));
});

Expand Down
4 changes: 2 additions & 2 deletions packages/service-worker/test/integration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ const serverUpdate =
scope.clients.getMock('default') !.queue.subscribe(msg => { mock.sendMessage(msg); });

mock.messages.subscribe(msg => { scope.handleMessage(msg, 'default'); });
mock.messagesClicked.subscribe(msg => { scope.handleMessage(msg, 'default'); });
mock.notificationClicks.subscribe(msg => { scope.handleMessage(msg, 'default'); });

mock.setupSw();
reg = mock.mockRegistration !;
Expand Down Expand Up @@ -135,7 +135,7 @@ const serverUpdate =
scope.updateServerState(serverUpdate);

const gotNotificationClick = (async() => {
const event: any = await obsToSinglePromise(push.messagesClicked);
const event: any = await obsToSinglePromise(push.notificationClicks);
expect(event.action).toEqual('clicked');
expect(event.notification.title).toEqual('This is a test');
})();
Expand Down
2 changes: 1 addition & 1 deletion packages/service-worker/testing/mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class MockServiceWorkerContainer {
mockRegistration: MockServiceWorkerRegistration|null = null;
controller: MockServiceWorker|null = null;
messages = new Subject();
messagesClicked = new Subject();
notificationClicks = new Subject();

addEventListener(event: 'controllerchange'|'message', handler: Function) {
if (event === 'controllerchange') {
Expand Down
24 changes: 12 additions & 12 deletions packages/service-worker/worker/src/driver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,8 @@ const IDLE_THRESHOLD = 5000;
const SUPPORTED_CONFIG_VERSION = 1;

const NOTIFICATION_OPTION_NAMES = [
'actions', 'badge', 'body', 'dir', 'icon', 'lang', 'renotify', 'requireInteraction', 'tag',
'vibrate', 'data'
];
const NOTIFICATION_CLICK_OPTION_NAMES = [
'actions', 'badge', 'title', 'body', 'dir', 'icon', 'image', 'lang', 'renotify',
'requireInteraction', 'tag', 'timestamp', 'vibrate', 'data'
'actions', 'badge', 'body', 'data', 'dir', 'icon', 'image', 'lang', 'renotify',
'requireInteraction', 'silent', 'tag', 'timestamp', 'title', 'vibrate'
];

interface LatestEntry {
Expand Down Expand Up @@ -188,8 +184,10 @@ export class Driver implements Debuggable, UpdateSource {
return;
}

// When opening DevTools in Chrome, a request is made for the current URL (and possibly related
// resources, e.g. scripts) with `cache: 'only-if-cached'` and `mode: 'no-cors'`. These request
// When opening DevTools in Chrome, a request is made for the current URL (and possibly
// related
// resources, e.g. scripts) with `cache: 'only-if-cached'` and `mode: 'no-cors'`. These
// request
// will eventually fail, because `only-if-cached` is only allowed to be used with
// `mode: 'same-origin'`.
// This is likely a bug in Chrome DevTools. Avoid handling such requests.
Expand Down Expand Up @@ -300,8 +298,8 @@ export class Driver implements Debuggable, UpdateSource {
notification.close();

const desc = notification as any;
let options: {[key: string]: string | undefined} = {};
NOTIFICATION_CLICK_OPTION_NAMES.filter(name => desc.hasOwnProperty(name))
const options: {[key: string]: string} = {};
NOTIFICATION_OPTION_NAMES.filter(name => desc.hasOwnProperty(name))
.forEach(name => options[name] = desc[name]);

await this.broadcast({
Expand Down Expand Up @@ -363,12 +361,14 @@ export class Driver implements Debuggable, UpdateSource {
// this.initialized will be `null` if initialization has not yet been attempted, or will be a
// Promise which will resolve (successfully or unsuccessfully) if it has.
if (this.initialized === null) {
// Initialization has not yet been attempted, so attempt it. This should only ever happen once
// Initialization has not yet been attempted, so attempt it. This should only ever happen
// once
// per SW instantiation.
this.initialized = this.initialize();
}

// If initialization fails, the SW needs to enter a safe state, where it declines to respond to
// If initialization fails, the SW needs to enter a safe state, where it declines to respond
// to
// network requests.
try {
// Wait for initialization.
Expand Down
4 changes: 2 additions & 2 deletions tools/public_api_guard/service-worker/service-worker.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ export declare class ServiceWorkerModule {
export declare class SwPush {
readonly isEnabled: boolean;
readonly messages: Observable<object>;
readonly messagesClicked: Observable<{
readonly notificationClicks: Observable<{
action: string;
notification: NotificationObject;
notification: NotificationObject & { title: string };
}>;
readonly subscription: Observable<PushSubscription | null>;
constructor(sw: NgswCommChannel);
Expand Down

0 comments on commit 9db790a

Please sign in to comment.