feat: Added reload from the terminal by pressing a key #746

saintsebastian · 2017-01-11T18:00:48Z

Fixes #705

Would love to here ideas about testing, my idea is mocking stdin, but I am open to suggestions.

coveralls · 2017-01-14T16:46:21Z

Coverage remained the same at 100.0% when pulling 0d81e24 on saintsebastian:feat-705 into 7d20a3c on mozilla:master.

coveralls · 2017-01-24T13:24:30Z

Coverage remained the same at 100.0% when pulling 5113e11 on saintsebastian:feat-705 into 7d20a3c on mozilla:master.

rpl

Hi @saintsebastian,
I added an initial set of review comments about this PR,
most of it looks nice, but there are some changes needed (e.g. related to a flow failure workaround that is currently used in this version of the PR, the testing strategy).

Also, as a "note to self" (and to @kumar303), it seems that process.stdin.isTTY has some issues on windows (e.g. like described in this nodejs issue nodejs/node#10238) and winpty looks like the most common suggested workaround to this issue (and so we will need to look into this a bit more deeply before landing this feature).

rpl · 2017-01-30T16:05:46Z

src/cmd/run.js

 |};

-


please leave this empty line (the two empty lines are currently used as a separator between "sections" of the source file)

rpl · 2017-01-30T16:08:10Z

src/cmd/run.js

 export type WatcherCreatorFn = (params: WatcherCreatorParams) => Watchpack;

 export function defaultWatcherCreator(
  {
    addonId, client, sourceDir, artifactsDir,
    onSourceChange = defaultSourceWatcher,
-    desktopNotifications = defaultDesktopNotifications,
+    reloader = reloadFn,


I would probably prefer a slightly more descriptive name (e.g. how about addonReload?)

rpl · 2017-01-30T16:17:07Z

src/cmd/run.js

@@ -256,6 +273,19 @@ export default async function run(
        );
      }

+      readline.emitKeypressEvents(process.stdin);
+      if (process.stdin.isTTY) {


I think that it would make sense to do not register the process.stdin.on('keypress') event listener if the process.stdin is not a tty.

rpl · 2017-01-30T16:18:50Z

src/cmd/run.js

@@ -256,6 +273,19 @@ export default async function run(
        );
      }

+      readline.emitKeypressEvents(process.stdin);
+      if (process.stdin.isTTY) {
+        log.info('Press R to reload');


we should also let the user know that we also support a keyboard shortcut for exiting the tool. (e.g. Press R to reload (and Ctrl-C to quit));

rpl · 2017-01-30T18:20:03Z

tests/unit/test-cmd/test.run.js

+
+  it('exits when user presses CTRL+C in shell console', () => {
+    const cmd = prepareRun();
+    const exits = sinon.stub(process, 'exit', sinon.spy(() => {}));


stubbing process.exit concerns me a bit, I know that it is supposed to be restored by the after hook but I would prefer if we could avoid this.

e.g. There is nothing in our code that currently exit in a arbitrary part of the sources using process.exit, it can be probably reasonable to solve this by:

creating a new utility method (e.g. exitProgram or exitWebExt) that incapsulate the strategy that we are going to use to exit the application (and it should print a log message to let the user know that web-ext is exiting because the user has requested it to)

inject the new utility method in the command run entrypoint function, by setting its default value to the utility method defined above

override the new utility method in the tests using sinon.spy

I'm also wondering if process.exit() is the best strategy to exit the application.

rpl · 2017-01-30T19:25:40Z

src/cmd/run.js

+      readline.emitKeypressEvents(process.stdin);
+      if (process.stdin.isTTY) {
+        log.info('Press R to reload');
+        (process.stdin: any).setRawMode(true);


This (process.stdin: any).setRawMode(...) looks like something that is hiding a flow validation error, am I right?

we should try to fix the flow validation error properly (and not by using the any type workaround which is too broad), e.g. it seems that the issue could be that flow is not able to know if process.stdin is going to be a tty.ReadStream (https://nodejs.org/api/tty.html#tty_class_tty_readstream), and so my guess is that something like this should make flow happy:

if (process.stdin instanceof tty.ReadStream) { process.stdin.setRawMode(true); }

rpl · 2017-01-30T19:45:59Z

src/cmd/run.js

@@ -256,6 +273,19 @@ export default async function run(
        );
      }

+      readline.emitKeypressEvents(process.stdin);


This would be probably better moved inside the if (process.stdin.isTTY) { ... } below.

coveralls · 2017-01-31T15:02:29Z

Coverage remained the same at 100.0% when pulling b6e110b on saintsebastian:feat-705 into 7d20a3c on mozilla:master.

saintsebastian · 2017-01-31T15:15:08Z

@rpl the issue you've mentioned may cause troubles, is there some way (automated testing, virtual machine) i can use to make sure this feature actually works outside of my system?

coveralls · 2017-01-31T20:51:39Z

Coverage remained the same at 100.0% when pulling 12b8c4f on saintsebastian:feat-705 into 7d20a3c on mozilla:master.

rpl · 2017-02-06T19:29:43Z

@rpl the issue you've mentioned may cause troubles, is there some way (automated testing, virtual machine) i can use to make sure this feature actually works outside of my system?

@saintsebastian I just tried on a Windows7 system and it worked correctly (with the interactive reload and exit on Ctrl-C as on OSX and Linux) with both cmd and ConEmu.

@saintsebastian currently there are conflicts with master (related to PR #753 landed recently), do you mind to merge the updated master into your branch to resolve the conflicts?
(the final "squash and merge" will squash all the commit from this PR into a single commit and so the merge commit will not be visible in the master history)

coveralls · 2017-03-10T17:42:52Z

Coverage remained the same at 100.0% when pulling a8e07e1 on saintsebastian:feat-705 into 5ba7e54 on mozilla:master.

coveralls · 2017-03-10T17:52:43Z

Coverage remained the same at 100.0% when pulling a02d4dd on saintsebastian:feat-705 into 5ba7e54 on mozilla:master.

rpl · 2017-03-10T18:18:35Z

@kumar303 this PR is now ready for a final look:
This change has presented some interesting challenges (e.g. we are introducing user interaction through the stdin stream for the first time, we had to carefully handle any risk of cross-"operating systems" compatibility issues, the fake test helper presented an interesting nodejs crash on travis, which has been currently fixed by tweaking the test helper, and more :-)) and so it is one of those changes where I'd like to get one additional round of review from you before landing it.

@saintsebastian Thank you so much for all the passion, hard work (and patience :-)) that you have put in it during the past reviews on this PR! 👍

saintsebastian · 2017-03-10T18:19:40Z

@rpl this was lots of fun! thanks for sticking with me all along!

…t-705

kumar303

Hi @saintsebastian, thanks for your patience :) I got caught up in deploying the new addons.mozilla.org mobile site but I have a bit more time now.

I requested some architectural changes but didn't get a chance to fully review the tests. When you're ready for a re-review, just select my name in the box so that I see the notification.

kumar303 · 2017-03-29T20:44:57Z

src/cmd/run.js

+  }: ReloadParams
+): Promise<void> {
+  log.debug(`Reloading add-on ID ${addonId}`);
+  return client.reloadAddon(addonId)


Is there any reason why this is using promise code instead of async/await code? I think it would read better as an async function with a try/catch.

kumar303 · 2017-03-29T20:46:29Z

src/cmd/run.js

 |};

 export type CmdRunOptions = {|
  firefoxApp: typeof defaultFirefoxApp,
  firefoxClient: typeof defaultFirefoxClient,
  reloadStrategy: typeof defaultReloadStrategy,
+  addonReload: typeof defaultAddonReload,
+  AddonRunner: typeof ExtensionRunner


nit: we typically use trailing commas to make refactoring easier but eslint doesn't enforce it in flow annotations.

kumar303 · 2017-03-29T20:49:30Z

src/cmd/run.js

 |};

 export default async function run(
  {
    sourceDir, artifactsDir, firefox, firefoxProfile,
    keepProfileChanges = false, preInstall = false, noReload = false,
    browserConsole = false, customPrefs, startUrl, ignoreFiles,
+    stdin = process.stdin,


In command handler functions, all of these parameters should correlate to command line options. Thus, stdin should be moved a couple lines down to the CmdRunOptions object.

kumar303 · 2017-03-29T20:54:04Z

src/cmd/run.js

 |};

 export type CmdRunOptions = {|
  firefoxApp: typeof defaultFirefoxApp,
  firefoxClient: typeof defaultFirefoxClient,
  reloadStrategy: typeof defaultReloadStrategy,
+  addonReload: typeof defaultAddonReload,
+  AddonRunner: typeof ExtensionRunner


How about calling this ExtensionRunnerClass? I know the word "addon" has crept into the code quite a bit but I've been trying to call them extensions for our theoretical and optimistic future compatibility with Chrome :-)

kumar303 · 2017-03-29T21:48:11Z

src/cmd/run.js

  }: CmdRunParams,
  {
    firefoxApp = defaultFirefoxApp,
    firefoxClient = defaultFirefoxClient,
    reloadStrategy = defaultReloadStrategy,
+    addonReload = defaultAddonReload,


I think it will be easier to move the new stdin loop logic to defaultReloadStrategy since that already is doing something very similar which is starting a loop to watch for source file changes.

Here is a quick patch to illustrate what I mean:

diff --git a/src/cmd/run.js b/src/cmd/run.js index cdeab34..a524a25 100644 --- a/src/cmd/run.js +++ b/src/cmd/run.js @@ -46,17 +46,16 @@ export type WatcherCreatorParams = {| onSourceChange?: OnSourceChangeFn, ignoreFiles?: Array<string>, createFileFilter?: FileFilterCreatorFn, - addonReload?: typeof defaultAddonReload, + addonReload: typeof defaultAddonReload, |}; export type WatcherCreatorFn = (params: WatcherCreatorParams) => Watchpack; export function defaultWatcherCreator( { - addonId, client, sourceDir, artifactsDir, ignoreFiles, + addonId, addonReload, client, sourceDir, artifactsDir, ignoreFiles, onSourceChange = defaultSourceWatcher, createFileFilter = defaultFileFilterCreator, - addonReload = defaultAddonReload, }: WatcherCreatorParams ): Watchpack { const fileFilter = createFileFilter( @@ -109,6 +108,7 @@ export type ReloadStrategyParams = {| |}; export type ReloadStrategyOptions = {| + addonReload?: typeof defaultAddonReload, createWatcher?: WatcherCreatorFn, createFileFilter?: FileFilterCreatorFn, |}; @@ -119,18 +119,20 @@ export function defaultReloadStrategy( sourceDir, artifactsDir, ignoreFiles, }: ReloadStrategyParams, { + addonReload = defaultAddonReload, createWatcher = defaultWatcherCreator, }: ReloadStrategyOptions = {} ): void { - const watcher: Watchpack = ( - createWatcher({addonId, client, sourceDir, artifactsDir, ignoreFiles}) - ); + const watcher: Watchpack = createWatcher({ + addonId, addonReload, client, sourceDir, artifactsDir, ignoreFiles, + }); firefoxProcess.on('close', () => { client.disconnect(); watcher.close(); }); + // Do the while (!userExit) loop right here... }

kumar303 · 2017-03-29T22:19:54Z

src/cmd/run.js

+        Promise.resolve().then(async function() {
+          log.info('Press R to reload (and Ctrl-C to quit)');
+
+          let userExit = false;


To expand on my comment above about moving this logic to defaultReloadStrategy, you're going to need to handle the case where Firefox quits. In other words, try these steps:

Execute web-ext run

Let Firefox open up

Go to File > Quit in the Firefox application to close it

After this, web-ext should stop running the while loop to check stdin for keyboard input but currently it does not. If you move your logic to defaultReloadStrategy then you can hook something into the already existing close event:

firefoxProcess.on('close', () => { client.disconnect(); watcher.close(); // stop polling stdin... });

saintsebastian · 2017-04-12T19:21:02Z

@kumar303 hey Kumar! i think this is ready for further review

coveralls · 2017-04-12T19:34:13Z

Coverage remained the same at 100.0% when pulling 877bec1 on saintsebastian:feat-705 into 5ba7e54 on mozilla:master.

kumar303

Thanks for the changes. The new architecture looks good, here are a few requests.

kumar303 · 2017-04-12T21:47:40Z

tests/unit/test-cmd/test.run.js

    }

    function prepare() {
      const client = new RemoteFirefox(fakeFirefoxClient());
      const watcher = {
        close: sinon.spy(() => {}),
      };
+      const fakeStdin = new tty.ReadStream();


I'd like to see the default fakeStdin set to something that has stdin.isTTY == false because that will prevent the while loop from getting triggered on every test of this function. For the tests that need to trigger the while loop, you can just pass in the tty.ReadStream() object as an explicit option.

kumar303 · 2017-04-12T21:58:28Z

src/cmd/run.js

+
+      let userExit = false;
+
+      while (!userExit) {


Some quick console logging tells me that this while loop never exits in about four tests. Since the test suite itself runs in a loop (when files change), this will produce a memory leak but perhaps it's insignificant. I mentioned below that you can fix some defaults to prevent most tests from entering this while loop. Maybe that change will be enough. If it's not too difficult, could you make sure that every test causes this while loop to exit?

@Kumar thanks for catching this, indeed defaults prevent the loop from starting, so it only happens in two tests, in one it doesn't exit since it is waiting for possible input after reload in another it exits on ctrl+c. Hope I understood your correctly and implemented this the way you imagined

kumar303 · 2017-04-12T22:39:58Z

src/cmd/run.js

+
+        if (keyPressed.ctrl && keyPressed.name === 'c') {
+          userExit = true;
+        } else if (keyPressed.name === 'r' && addonId) {


I think it would be nice to log a debug message here so that we know the reload was triggered by a key press

kumar303 · 2017-04-12T22:50:47Z

tests/unit/test.program.js

@@ -280,7 +285,7 @@ describe('program.main', () => {
      getVersion: () => 'not-a-real-version',
      checkForUpdates: spy(),
      shouldExitProgram: false,
-      systemProcess: fake(process),
+      systemProcess: createFakeProcess,


I think was supposed to be createFakeProcess(), right?

saintsebastian · 2017-04-13T19:41:37Z

@kumar303 thanks for catching all these things!

coveralls · 2017-04-13T19:47:46Z

Coverage remained the same at 100.0% when pulling 68089e9 on saintsebastian:feat-705 into 5ba7e54 on mozilla:master.

kumar303

Thanks, this looks great. I only have some minor cleanup requests.

kumar303 · 2017-04-14T14:46:49Z

src/cmd/run.js

+        if (keyPressed.ctrl && keyPressed.name === 'c') {
+          userExit = true;
+        } else if (keyPressed.name === 'r' && addonId) {
+          log.info('\nReloading extension on user request');


This should be log.debug() because we only want it to be printed in --verbose mode. Since we don't reset the cursor in verbose mode, you won't need a \n (new line) in the message.

kumar303 · 2017-04-14T14:49:28Z

src/cmd/run.js

+    stdin.setRawMode(true);
+
+    // NOTE: this `Promise.resolve().then(...)` is basically used to spawn a "co-routine" that is executed
+    // before the callback attached to the Promise returned by this function (and it allows the `run` function


I think it's no longer accurate to say this is "executed before the callback attached to the Promise returned by this function..." (since the function no longer returns a promise, right?) -- could you update the comment? It's important to get this comment right because I had to read it several times to understand what this code is doing :)

kumar303 · 2017-04-14T14:52:30Z

tests/unit/test-cmd/test.run.js

        createWatcher: sinon.spy(() => watcher),
+        stdin: fakeStdin,


this could just be: stdin: new stream.Readable(),

saintsebastian · 2017-04-14T15:06:04Z

@kumar303 thanks for such a fast review! Hopefully most of the work is done and I can replicate what I've learnt here in other PRs.

coveralls · 2017-04-14T15:08:36Z

Coverage remained the same at 100.0% when pulling 14f4a8c on saintsebastian:feat-705 into 5ba7e54 on mozilla:master.

kumar303

Let's do it!

muffinresearch added the state: pull request ready label Jan 11, 2017

saintsebastian requested a review from rpl January 12, 2017 15:28

saintsebastian assigned rpl Jan 12, 2017

saintsebastian added 6 commits January 13, 2017 22:00

fix: issue 510 new error type

36e9947

fix: issue 510 second attempt

8c93bbf

fix: issue 510 test and better error handling

a50013a

fix: issue 510 more tests

5dc6098

fix: additional behaviours with tests

1d75365

test: imports for errorWithCode

bf35ff6

test: replaces injections with real directories

de75bcf

saintsebastian added 2 commits January 25, 2017 18:47

feat: testing skipped for windows

bc924de

feat: initital

af979c9

rpl changed the title ~~feat: issue 705 new reload function~~ feat: Added reload from the terminal by pressing a key Jan 30, 2017

rpl requested changes Jan 30, 2017

View reviewed changes

saintsebastian added 6 commits February 7, 2017 16:37

fix: merging upstream

cc1b862

feat: issue 705 new reload function

0805d20

test: process keypress events in run

0d56e0f

test: after is added

953761e

feat: exit function

4f3fa4b

fix: optional parameter for defaultExitProgram

b78ca40

saintsebastian force-pushed the feat-705 branch from 12b8c4f to 0b1c2f8 Compare February 7, 2017 15:54

saintsebastian closed this Feb 7, 2017

saintsebastian reopened this Feb 7, 2017

muffinresearch removed the state: pull request ready label Feb 7, 2017

saintsebastian added 2 commits March 10, 2017 18:47

test: better tests

a02d4dd

test: testing addon reload

a46281f

rpl approved these changes Mar 10, 2017

View reviewed changes

rpl assigned kumar303 Mar 10, 2017

rpl requested a review from kumar303 March 10, 2017 18:15

Merge branch 'feat-705' of github.com:saintsebastian/web-ext into fea…

2a0994d

…t-705

rpl mentioned this pull request Mar 24, 2017

feat: web-ext create simple version #771

Closed

kumar303 suggested changes Mar 29, 2017

View reviewed changes

feat: interactive loop and tests modified

7dbb6ee

saintsebastian requested a review from kumar303 April 12, 2017 19:21

ammend

877bec1

kumar303 suggested changes Apr 12, 2017

View reviewed changes

fix: endless loop in tests

68089e9

saintsebastian requested a review from kumar303 April 13, 2017 19:41

kumar303 suggested changes Apr 14, 2017

View reviewed changes

fix: cleanup

14f4a8c

saintsebastian requested a review from kumar303 April 14, 2017 15:06

kumar303 approved these changes Apr 14, 2017

View reviewed changes

kumar303 merged commit 97404a9 into mozilla:master Apr 14, 2017

kumar303 removed the state: pull request ready label Apr 14, 2017

feat: Added reload from the terminal by pressing a key #746

feat: Added reload from the terminal by pressing a key #746

Conversation

saintsebastian commented Jan 11, 2017 • edited Loading

coveralls commented Jan 14, 2017 • edited Loading

coveralls commented Jan 24, 2017 • edited Loading

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 31, 2017 • edited Loading

saintsebastian commented Jan 31, 2017

coveralls commented Jan 31, 2017 • edited Loading

rpl commented Feb 6, 2017

coveralls commented Mar 10, 2017 • edited Loading

coveralls commented Mar 10, 2017 • edited Loading

rpl commented Mar 10, 2017

saintsebastian commented Mar 10, 2017

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saintsebastian commented Apr 12, 2017

coveralls commented Apr 12, 2017 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saintsebastian commented Apr 13, 2017

coveralls commented Apr 13, 2017 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saintsebastian commented Apr 14, 2017

coveralls commented Apr 14, 2017 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

saintsebastian commented Jan 11, 2017 •

edited

Loading

coveralls commented Jan 14, 2017 •

edited

Loading

coveralls commented Jan 24, 2017 •

edited

Loading

coveralls commented Jan 31, 2017 •

edited

Loading

coveralls commented Jan 31, 2017 •

edited

Loading

coveralls commented Mar 10, 2017 •

edited

Loading

coveralls commented Mar 10, 2017 •

edited

Loading

coveralls commented Apr 12, 2017 •

edited

Loading

coveralls commented Apr 13, 2017 •

edited

Loading

coveralls commented Apr 14, 2017 •

edited

Loading