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

feat: show desktop notifications on addon reload #674

Merged
merged 8 commits into from Jan 5, 2017

Conversation

Projects
None yet
5 participants
@saintsebastian
Copy link
Collaborator

commented Dec 14, 2016

I've installed a package which seemed well thought-through and maintained. However, I am not sure about how effective catching the error is currently.

fixes #511

@rpl
Copy link
Member

left a comment

Hi @saintsebastian,
I agree node-notifier seems to be a great pick!

I added some comment on the current code, but one more thing that it would be nice to add to this change is "testing that we are actually sending the desktop notifications" and it will require some change to the current approach:

a lot of the internals take optional parameters/options that are used in the unit tests to inject sinon stubs/spies (e.g. the unit tests related to the defaultWatcherCreator we use this approach to create a fake RemoteFirefox instance here: https://github.com/saintsebastian/web-ext/blob/5ab8ae80ef7efab58f06e9d658f245749022eec0/tests/unit/test-cmd/test.run.js#L236-L238), and we can use the same strategy for the desktop notification:

  • in the web-ext sources we have to change the WatcherCreatorParams type and defaultWatcherCreator helper function definitions so that they include the notifier instance as a parameter to the defaultWatcherCreator (the parameter should have as its default value the instance created using the real NotificationCenter constructor)
  • in the unit tests we can inject a fake NotificationCenter instance and then we can use it to check that it is used as expected in the error and success scenarios

There is currently a small conflict on this pull request (in the package.json file), but it can be easily solved by rebasing the branch on a recent master or by merging a recent master in your branch if you prefer (I usually prefer to rebase my changes when there is a conflict between the changes that I've submitted in a pull request and the changes landed on master in the meantime because it helps to keep my feature branch as small and simple as possible).

@@ -49,9 +50,20 @@ export function defaultWatcherCreator(
return onSourceChange({
sourceDir, artifactsDir, onChange: () => {
log.debug(`Reloading add-on ID ${addonId}`);
var notifier = new NC({

This comment has been minimized.

Copy link
@rpl

rpl Dec 15, 2016

Member

notifier will never be re-assigned after being created, and so it is a perfect candidate to be turned into a const.
(also we should use let instead of var, we do not have the no-var eslint rule yet, but we are going to introduce it soon)

This comment has been minimized.

Copy link
@rpl

rpl Dec 15, 2016

Member

This onChange callback is called every time a file changes, and so it would be better to move the code that creates the notifier instance elsewhere, so that we can be sure that we create it only once (e.g. we can create the instance near where const log = createLogger(...) is currently defined).

@coveralls

This comment has been minimized.

Copy link

commented Dec 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling ac3fd04 on saintsebastian:fix-511 into 9643db5 on mozilla:master.

@saintsebastian saintsebastian force-pushed the saintsebastian:fix-511 branch from ac3fd04 to 15162aa Dec 16, 2016

@coveralls

This comment has been minimized.

Copy link

commented Dec 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 15162aa on saintsebastian:fix-511 into 88a8a04 on mozilla:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

commented Dec 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 15162aa on saintsebastian:fix-511 into 88a8a04 on mozilla:master.

@rpl
Copy link
Member

left a comment

Hi @saintsebastian
This PR looks almost good, the following detailed comments are mainly related to some additional refactoring and minor cleanups (e.g. the bigger change is about moving the direct usage of the external dependency into a separate utility module).

This pull request also contains some conflicts on the module 'src/cmd/run.js' (which has been changed on master by your changes landed as part of #658 and the changes related to the new imports/exports eslint rules landed as part of #681), and so it will need a rebase on top of those changes.

Let me know if the comments related to the refactoring are not detailed enough, or if you want to discuss any of the other comments before applying the related changes.

@@ -9,9 +9,13 @@ import {
import {createLogger} from '../util/logger';
import getValidatedManifest, {getManifestId} from '../util/manifest';
import defaultSourceWatcher from '../watcher';
import {NotificationCenter as NC} from 'node-notifier';

This comment has been minimized.

Copy link
@rpl

rpl Dec 22, 2016

Member

I know that NotificationCenter is a pretty verbose name, but given that we use this identifier only once, I would prefer to use it without aliasing it to NC.

@@ -15,6 +15,7 @@ import {RemoteFirefox} from '../../../src/firefox/remote';
import {TCPConnectError, fakeFirefoxClient, makeSureItFails, fake, fixturePath}
from '../helpers';
import {createLogger} from '../../../src/util/logger';
import {NotificationCenter as NC} from 'node-notifier';

This comment has been minimized.

Copy link
@rpl

rpl Dec 22, 2016

Member

no need to import the real NotificationCenter identifier here in the tests, we can fake it by creating a simple object with a notify method (more details in the comment that follows).

@@ -240,6 +241,9 @@ describe('run', () => {
sourceDir: '/path/to/extension/source/',
artifactsDir: '/path/to/web-ext-artifacts',
onSourceChange: sinon.spy(() => {}),
messenger: fake(NC.prototype, {

This comment has been minimized.

Copy link
@rpl

rpl Dec 22, 2016

Member

This should be possible even without the NotificationCenter.prototype, e.g.

const fakeDesktopNotifications = {
  notify: sinon.spy(() => Promise.resolve()),
};
...
}: WatcherCreatorParams
): Watchpack {
return onSourceChange({
sourceDir,
artifactsDir,
onChange: () => {
log.debug(`Reloading add-on ID ${addonId}`);
messenger.notify({
title: 'Started reloading',

This comment has been minimized.

Copy link
@rpl

rpl Dec 22, 2016

Member

How about tweaking these titles so that the source of the notification is more easily recognizable?
(e.g. by prefixing the title with something like "web-ext run: ").

@@ -36,6 +40,7 @@ export type WatcherCreatorParams = {
sourceDir: string,
artifactsDir: string,
onSourceChange?: OnSourceChangeFn,
messenger?: NC,

This comment has been minimized.

Copy link
@rpl

rpl Dec 22, 2016

Member

How about renaming this property to desktopNotifications or something similar?

It would also be better if we move the direct usage of the external dependency (node-notifier) into an utility class or function in a new utility module (e.g. '../util/desktop-notifications'), and then we can create an additional test case for our internal utility code, which tests that it actually calls the external dependency with the expected parameters, in addition to the test cases already included in this pull request (which is testing that the watcher will show the expected notifications in the error and success scenarios).

This is almost what we are doing in #676 for the external dependency 'update-notifier'.

messenger.notify({
title: 'Started reloading',
message: `Reloading add-on ID ${addonId}`,
});
return client.reloadAddon(addonId)
.catch((error) => {

This comment has been minimized.

Copy link
@rpl

rpl Dec 22, 2016

Member

It would be probably better to notify the user that the addon has been reloaded successfully (the success scenario) instead of sending a notification before starting the reload, so that we alway show 1 notification per addon reload (with the current implementation we are going to show 1 notification when the addon reload successfully and 2 notifications on reloading errors).

We can achieve it by running the success notify in a then callback before the above catch, eg.

return client.reloadAddon(addonId)
  .then(() => {
     // notify addon successfully reloaded! 
     // this is only called if the promise returned by reloadAddon has not been rejected
  })
  .catch((error) => {
     // notify addon reloading error! 
     // this is called when the promise returned by reloadAddon has been rejected, or if the above callback
     // raises an exception.
  });

@rpl rpl changed the title Fix: issue 511 notifications on reload feat: show desktop notifications on addon reload Dec 22, 2016

saintsebastian added some commits Dec 14, 2016

fix: issue 511 notifications tested
fix: issue 511 added fallback option

fix: issue 511 added error message

@saintsebastian saintsebastian force-pushed the saintsebastian:fix-511 branch from 15162aa to 0bc8af1 Dec 22, 2016

@coveralls

This comment has been minimized.

Copy link

commented Dec 22, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling c6de8f6 on saintsebastian:fix-511 into 183ada5 on mozilla:master.

return client.reloadAddon(addonId)
.then(() => {
desktopNotifications({

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 22, 2016

Member

Hi all, just catching up with this.

I really love the idea of seeing a notification on error but showing a notification every time it reloads seems a bit much to me. I think it would minimize the effect of seeing a notification for failures (i.e. you might start to ignore the notices).

Do you really think we need to show a notice on every reload? If you do, I would request not to make this the default behavior but to allow it with a new option: --notify-on-reload. I can see how reload notifications might be useful to trust that the system is running.

This comment has been minimized.

Copy link
@saintsebastian

saintsebastian Dec 22, 2016

Author Collaborator

I like this idea! @rpl what do you think?

This comment has been minimized.

Copy link
@rpl

rpl Dec 22, 2016

Member

@saintsebastian 👍 yeah, I think that I agree with @kumar303 on this, by default we should only show a desktop notification when there is an error.

(I would be ok to enable the 'desktop notification on success' with an additional cli option, but we can also keep this PR simple and evaluate to introduce the additional cli option and evaluate its correct behavior in a follow up)

This comment has been minimized.

Copy link
@kumar303

kumar303 Dec 22, 2016

Member

Here are some other ideas to keep this patch simple so it's easier to land:

  • Only show a reload notification in verbose mode
  • Remove the reload notification entirely and add it in later once we have a command line option

This comment has been minimized.

Copy link
@saintsebastian

saintsebastian Dec 25, 2016

Author Collaborator

@rpl @kumar303 i've removed notification on reload and corresponding test for now, once the code is reviewed and decision is made on how to better handle it, we can continue working on it.

@coveralls

This comment has been minimized.

Copy link

commented Dec 25, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 3ce6039 on saintsebastian:fix-511 into 31d2e46 on mozilla:master.

@rpl
Copy link
Member

left a comment

@saintsebastian My apologies for the long time I needed to get back to this PR, Follow some comments related to some small tweaks.

@@ -57,6 +57,8 @@
"minimatch": "3.0.3",
"mz": "2.6.0",
"node-firefox-connect": "1.2.0",
"node-notifier": "4.6.1",
"regenerator-runtime": "0.10.1",

This comment has been minimized.

Copy link
@rpl

rpl Jan 2, 2017

Member

remove this line (regenerator runtime is already included in the dependencies at line 63)

@@ -2,6 +2,7 @@
import type FirefoxProfile from 'firefox-profile';
import type Watchpack from 'watchpack';

import {desktopNotifications as notifier} from '../util/desktop-notifier';

This comment has been minimized.

Copy link
@rpl

rpl Jan 2, 2017

Member

how about aliasing this as defaultDesktopNotifications instead of notifier? (like we do for defaultFirefoxConnector at line 7)

});

type desktopNotificationsParams = {
titleString: string,

This comment has been minimized.

Copy link
@rpl

rpl Jan 2, 2017

Member

let's rename notifierSource to notifier, titleString to title and messageString to message, it will also help to make it line 22 shorter like in:

notifier.notify({title, message});
type desktopNotificationsParams = {
titleString: string,
messageString: string,
notifierSource?: NotificationCenter,

This comment has been minimized.

Copy link
@rpl

rpl Jan 2, 2017

Member

this should be typeof NotificationCenter.

/* @flow */
import {NotificationCenter} from 'node-notifier';

const notifier = new NotificationCenter({

This comment has been minimized.

Copy link
@rpl

rpl Jan 2, 2017

Member

let's rename this to defaultNotifier

@rpl

This comment has been minimized.

Copy link
Member

commented Jan 2, 2017

It would be nice (even in a follow up of this PR) to include an icon into the desktop notification (e.g. the icon of the reloaded web-extensions)

@kumar303 how it sounds to you?

@saintsebastian

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 3, 2017

Thanks for the comments! I added icon option to desktop-notifier.jsso this should be very easy to implement as soon as it is decided what is the best way to represent reload (extension's own icon or a generic reload).

@coveralls

This comment has been minimized.

Copy link

commented Jan 3, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 15b5983 on saintsebastian:fix-511 into 31d2e46 on mozilla:master.

@kumar303
Copy link
Member

left a comment

This is a cool feature. I'll take a closer look next but here are some initial change requests:

  • I think you should use the default notifier instead of NotificationCenter because according to their docs this will be more cross-platform (Linux, Windows, Mac, and a fallback to Growl).
  • I'd like to see the error in the callback logged so that someone isn't surprised why their notifications aren't working

Here is a patch just to illustrate what I mean but the logging needs to be fixed up:

diff --git a/src/util/desktop-notifier.js b/src/util/desktop-notifier.js
index 794880b..33ce54a 100644
--- a/src/util/desktop-notifier.js
+++ b/src/util/desktop-notifier.js
@@ -1,15 +1,11 @@
 /* @flow */
-import {NotificationCenter} from 'node-notifier';
-
-const defaultNotifier = new NotificationCenter({
-  withFallback: true,
-});
+import defaultNotifier from 'node-notifier';
 
 type desktopNotificationsParams = {
   title: string,
   message: string,
   icon?: string,
-  notifier?: typeof NotificationCenter,
+  notifier?: typeof defaultNotifier,
 };
 
 export function desktopNotifications(
@@ -18,5 +14,7 @@ export function desktopNotifications(
   }: desktopNotificationsParams
 ): void {
 
-  notifier.notify({title, message, icon});
+  notifier.notify({title, message, icon}, (error, response) => {
+    console.log('notifier error:', error, 'response:', response);
+  });
 }
@kumar303

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

It would be nice (even in a follow up of this PR) to include an icon into the desktop notification

I don't think it's such a big deal but I'm not opposed to it.

title: string,
message: string,
icon?: string,
notifier?: typeof NotificationCenter,

This comment has been minimized.

Copy link
@kumar303

kumar303 Jan 4, 2017

Member

This shouldn't be part of the main params because it doesn't relate to the notification call itself. You should pass it in as a secondary option so that it won't ever collide with a real notification param (which is unlikely but this follows our pattern elsewhere in web-ext).

Example:

export function desktopNotifications(
  {
    title, message, icon,
  }: desktopNotificationsParams,
  {
    notifier = defaultNotifier,
  }: desktopNotificationOptions = {}
): void {
@saintsebastian

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 5, 2017

@kumar303 this is a great suggestion, but I had to employ a kind of work around to test it. Thanks for the review!

@coveralls

This comment has been minimized.

Copy link

commented Jan 5, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling b2915de on saintsebastian:fix-511 into 31d2e46 on mozilla:master.

@rpl
Copy link
Member

left a comment

@saintsebastian Hi!

Follows a bunch of comments with some suggestions (mostly related to flow types, coding style conventions and some questions about the logged errors):

notifier?: typeof NotificationCenter,
};

type desktopNotificationsOpts = {

This comment has been minimized.

Copy link
@rpl

rpl Jan 5, 2017

Member

we should probably call this type desktopNotificationsOptions (mostly because we already used the Options suffix for other similar types and it can be nicer if we use the same convention here)


type desktopNotificationsOpts = {
notifier?: typeof defaultNotifier,
logger?: typeof log,

This comment has been minimized.

Copy link
@rpl

rpl Jan 5, 2017

Member

you should be able to import and reuse the type defined from the util/logger module, something like:

import type {Logger} from "./logger";

...

type desktopNotificationOpts = {
   ...
   logger?: Logger,
}
notifier?: typeof NotificationCenter,
};

type desktopNotificationsOpts = {

This comment has been minimized.

Copy link
@rpl

rpl Jan 5, 2017

Member

we should export this type:

export type ...

it is a type used in an exported function, and so it is possible that we would need to import this type into another module.

}: desktopNotificationsParams,
{
notifier = defaultNotifier,
logger = log,

This comment has been minimized.

Copy link
@rpl

rpl Jan 5, 2017

Member

@kumar303 This solution to spy the logger seems reasonable to me, how it looks to you?

notifier.notify({title, message, icon});
notifier.notify({title, message, icon}, (err, res) => {
if (err) {
logger.debug(`notifier error: ${err.message},` +

This comment has been minimized.

Copy link
@rpl

rpl Jan 5, 2017

Member

It would be probably better to capitalize the first letter of this comment (e.g. Desktop notifier error: ...)

Also, I'm not sure if the log level is the right one, probably @kumar303 was suggesting to log the error even when web-ext is not running in verbose mode.

@kumar303 am I right?

notifier.notify({title, message, icon}, (err, res) => {
if (err) {
logger.debug(`notifier error: ${err.message},` +
` response: ${res}`);

This comment has been minimized.

Copy link
@rpl

rpl Jan 5, 2017

Member

What is res supposed to contains?
if it is an object, then this is going to probably log "[Object]", that is not exactly very much informative ;-)

This comment has been minimized.

Copy link
@saintsebastian

saintsebastian Jan 5, 2017

Author Collaborator

They are printed in console in the docs, so hopefully they should be readable

it('is called and creates a message with correct parameters', () => {
const fakeNotifier = {
notify: sinon.spy(() => Promise.resolve()),
};

desktopNotifications(

This comment has been minimized.

Copy link
@rpl

rpl Jan 5, 2017

Member

This could be better formatted as:

desktopNotification(expectedNotification, {
  ...
});

(also in the other places where there is a similar function call).

@kumar303
Copy link
Member

left a comment

Sorry about the nits! There is also a possible async/timing problem that needs addressing.

}: desktopNotificationsParams,
{
notifier = defaultNotifier,
logger = log,

This comment has been minimized.

Copy link
@kumar303

kumar303 Jan 5, 2017

Member

Just a nit but I think this should be log = defaultLog to follow the convention we use elsewhere in web-ext

logger?: typeof log,
};

export function desktopNotifications(

This comment has been minimized.

Copy link
@kumar303

kumar303 Jan 5, 2017

Member

nit: since this only does one thing, I think naming it showDesktopNotification makes more sense

}: desktopNotificationsOpts = {}
): void {

notifier.notify({title, message, icon}, (err, res) => {

This comment has been minimized.

Copy link
@kumar303

kumar303 Jan 5, 2017

Member

Logging the error looks good, thanks, but this introduces a synchronization problem for testing. You will need to return a promise instead (just for testing purposes), like:

return new Promise((resolve, reject) => {
  notifier.notify({title, message, icon}, (err, res) => {
    if (err) {
      logger.debug(`notifier error: ${err.message},` +
                   ` response: ${res}`);
      reject(err);
    } else {
      resolve();
    }
  });
});
},
};

desktopNotifications(

This comment has been minimized.

Copy link
@kumar303

kumar303 Jan 5, 2017

Member

After converting this function to a promise, you can do:

return desktopNotifications(...)
  .then(makeSureItFails())
  .catch(() => {
    assert.ok(log.debug.called);
  });

Without a promise, it's hard to guarantee that this test will run the same way every time (if, say, we refactored some of the desktopNotifications code).

const fakeNotifier = {
notify: sinon.spy(() => Promise.resolve()),
};
desktopNotifications(

This comment has been minimized.

Copy link
@kumar303

kumar303 Jan 5, 2017

Member

for consistency, you should probably also use this as a promise everywhere:

return desktopNotifications(...)
  .then(() => {
    assert.ok(fakeNotifier.notify.called);
  });

it('is called and creates a message with correct parameters', () => {
const fakeNotifier = {
notify: sinon.spy(() => Promise.resolve()),

This comment has been minimized.

Copy link
@kumar303

kumar303 Jan 5, 2017

Member

The signature that you need to mock looks more like this (according to their docs):

const fakeNotifier = {
  notify: sinon.spy((options, callback) => callback()),
};
});
assert.ok(log.debug.called);
assert.equal(log.debug.firstCall.args[0],
`notifier error: ${expectedError.message}, response: response`);

This comment has been minimized.

Copy link
@kumar303

kumar303 Jan 5, 2017

Member

nit: this should either line up with the inside of the opening parenthesis or be indented only two spaces

@coveralls

This comment has been minimized.

Copy link

commented Jan 5, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling cb9204f on saintsebastian:fix-511 into 31d2e46 on mozilla:master.

@saintsebastian

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 5, 2017

@kumar303 @rpl thanks for cleaning up this code and also clearing up some things for me in the process

@kumar303
Copy link
Member

left a comment

Thanks for all the fixups!

@rpl

rpl approved these changes Jan 5, 2017

Copy link
Member

left a comment

Looks great to me too!

@saintsebastian Thanks! I absolutely love this feature! ❤️

@kumar303

This comment has been minimized.

Copy link
Member

commented Jan 5, 2017

I'm super excited about this! I've stared at Firefox scratching my head more than once, only to discover a syntax error in the console.

@kumar303 kumar303 merged commit d29ca42 into mozilla:master Jan 5, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.