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: Added reload from the terminal by pressing a key #746

Merged
merged 40 commits into from
Apr 14, 2017
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
36e9947
fix: issue 510 new error type
saintsebastian Dec 26, 2016
8c93bbf
fix: issue 510 second attempt
saintsebastian Dec 28, 2016
a50013a
fix: issue 510 test and better error handling
saintsebastian Jan 5, 2017
5dc6098
fix: issue 510 more tests
saintsebastian Jan 5, 2017
1d75365
fix: additional behaviours with tests
saintsebastian Jan 13, 2017
bf35ff6
test: imports for errorWithCode
saintsebastian Jan 13, 2017
de75bcf
test: replaces injections with real directories
saintsebastian Jan 19, 2017
bc924de
feat: testing skipped for windows
saintsebastian Jan 25, 2017
af979c9
feat: initital
saintsebastian Jan 30, 2017
cc1b862
fix: merging upstream
saintsebastian Feb 7, 2017
0805d20
feat: issue 705 new reload function
saintsebastian Jan 11, 2017
0d56e0f
test: process keypress events in run
saintsebastian Jan 14, 2017
953761e
test: after is added
saintsebastian Jan 24, 2017
4f3fa4b
feat: exit function
saintsebastian Jan 31, 2017
b78ca40
fix: optional parameter for defaultExitProgram
saintsebastian Jan 31, 2017
fad5046
fix: merging conflicts
saintsebastian Feb 7, 2017
9a37106
feat: firefox killed before exit
saintsebastian Feb 14, 2017
13a4cc0
Merge branch 'master' into feat-705
saintsebastian Feb 14, 2017
b665d85
fix: type fix from master is applied
saintsebastian Feb 14, 2017
2f1156e
test: attempt to deal with flow errors
saintsebastian Feb 16, 2017
bc83865
fix: only exit on user request
saintsebastian Feb 16, 2017
91843f0
test: defaultExitProgramm
saintsebastian Feb 16, 2017
bd799fb
fix: pause input instead of exit
saintsebastian Feb 20, 2017
4390477
test: stdin test for defaultExitProgram
saintsebastian Feb 20, 2017
a08177e
fix: better handling of type check
saintsebastian Feb 24, 2017
b4b7a11
Merge branch 'master' into feat-705
saintsebastian Feb 24, 2017
f8b82da
feat: new implementation of exit strategy
saintsebastian Mar 6, 2017
d37e58c
Merge branch 'master' into feat-705
saintsebastian Mar 6, 2017
025d40a
fix: fixes in helper
saintsebastian Mar 9, 2017
c2fca58
fix: cleanup and nameing
saintsebastian Mar 9, 2017
cf326b5
test: testing addon realod
saintsebastian Mar 10, 2017
2e3e05c
test: removed fake from test cases
saintsebastian Mar 10, 2017
a8e07e1
test: test helper added
saintsebastian Mar 10, 2017
a02d4dd
test: better tests
saintsebastian Mar 10, 2017
a46281f
test: testing addon reload
saintsebastian Mar 10, 2017
2a0994d
Merge branch 'feat-705' of github.com:saintsebastian/web-ext into fea…
saintsebastian Mar 10, 2017
7dbb6ee
feat: interactive loop and tests modified
saintsebastian Apr 12, 2017
877bec1
ammend
saintsebastian Apr 12, 2017
68089e9
fix: endless loop in tests
saintsebastian Apr 13, 2017
14f4a8c
fix: cleanup
saintsebastian Apr 14, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
"minimatch": "3.0.3",
"mkdirp": "0.5.1",
"mz": "2.6.0",
"node-firefox-connect": "1.2.0",
"node-firefox-connect": "1.2.0",
"open": "0.0.5",
"node-notifier": "5.0.2",
"parse-json": "2.2.0",
Expand Down
85 changes: 68 additions & 17 deletions src/cmd/run.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
/* @flow */
import readline from 'readline';
import tty from 'tty';

import type FirefoxProfile from 'firefox-profile';
import type Watchpack from 'watchpack';

Expand Down Expand Up @@ -41,9 +44,9 @@ export type WatcherCreatorParams = {|
sourceDir: string,
artifactsDir: string,
onSourceChange?: OnSourceChangeFn,
desktopNotifications?: typeof defaultDesktopNotifications,
ignoreFiles?: Array<string>,
createFileFilter?: FileFilterCreatorFn,
addonReload?: typeof defaultAddonReload,
|};

export type WatcherCreatorFn = (params: WatcherCreatorParams) => Watchpack;
Expand All @@ -52,8 +55,8 @@ export function defaultWatcherCreator(
{
addonId, client, sourceDir, artifactsDir, ignoreFiles,
Copy link
Member

Choose a reason for hiding this comment

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

it looks like addonId and client are also unused in defaultWatcherCreator now (they are parameters of the addonReload)

Copy link
Contributor Author

@saintsebastian saintsebastian Feb 10, 2017

Choose a reason for hiding this comment

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

@rpl what's the best way to handle this in your opinion? Have a second set of types, so the function of both is clear like in defaultWatcherCreator({...WatcherCreatorParams}, {...ReloadParams}) or pass them like defaultWatcherCreator(addend, client, {...WatcherCreatorParams}) ?

Copy link
Member

Choose a reason for hiding this comment

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

@saintsebastian actually I think that we can leave it like it is right now

(initially I was thinking that we could pass an addonReload utility method that do not need to receive the addonId and the client from the defaultWatcherCreator, e.g. by capture them into a closure, which is absolutely possible, but after re-reading it again I changed my mind and I think that we can leave to defaultWatcherCreator the responsability of providing addonId and client to the addonReload function)

onSourceChange = defaultSourceWatcher,
desktopNotifications = defaultDesktopNotifications,
createFileFilter = defaultFileFilterCreator,
addonReload = defaultAddonReload,
}: WatcherCreatorParams
): Watchpack {
const fileFilter = createFileFilter(
Expand All @@ -62,24 +65,36 @@ export function defaultWatcherCreator(
return onSourceChange({
sourceDir,
artifactsDir,
onChange: () => {
log.debug(`Reloading add-on ID ${addonId}`);

return client.reloadAddon(addonId)
.catch((error) => {
log.error('\n');
log.error(error.stack);
desktopNotifications({
title: 'web-ext run: error occurred',
message: error.message,
});
throw error;
});
},
onChange: () => addonReload({addonId, client}),
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not wrong, these addonReload/ ReloadParams/ defaultAddonReload can be reverted: in the current version of this change, the addonReload helper seems to be no different from the previous version of this onChange callback.

Reverting these changes should help to greatly simplify this change.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I was wrong, I was forgetting that the addReload utility function is needed by the keypress handler.

@saintsebastian As per discussion over email, you can ignore this comment and only handle the rest of the review comments.

shouldWatchFile: (file) => fileFilter.wantFile(file),
});
}

export type ReloadParams = {|
addonId: string,
client: RemoteFirefox,
desktopNotifications?: typeof defaultDesktopNotifications,
|};

export function defaultAddonReload(
{
addonId, client,
desktopNotifications = defaultDesktopNotifications,
}: ReloadParams
): Promise<void> {
log.debug(`Reloading add-on ID ${addonId}`);
return client.reloadAddon(addonId)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

.catch((error) => {
log.error('\n');
log.error(error.stack);
desktopNotifications({
title: 'web-ext run: error occurred',
message: error.message,
});
throw error;
});
}


// defaultReloadStrategy types and implementation.

Expand Down Expand Up @@ -180,24 +195,30 @@ export type CmdRunParams = {|
customPrefs?: FirefoxPreferences,
startUrl?: string | Array<string>,
ignoreFiles?: Array<string>,
stdin: stream$Readable,
|};

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :-)

|};

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

Choose a reason for hiding this comment

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

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.

}: CmdRunParams,
{
firefoxApp = defaultFirefoxApp,
firefoxClient = defaultFirefoxClient,
reloadStrategy = defaultReloadStrategy,
addonReload = defaultAddonReload,
Copy link
Contributor

Choose a reason for hiding this comment

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

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...
 }
 
 

AddonRunner = ExtensionRunner,
}: CmdRunOptions = {}): Promise<Object> {

log.info(`Running web extension from ${sourceDir}`);
Expand All @@ -216,7 +237,7 @@ export default async function run(

const manifestData = await getValidatedManifest(sourceDir);

const runner = new ExtensionRunner({
const runner = new AddonRunner({
sourceDir,
firefoxApp,
firefox,
Expand Down Expand Up @@ -272,6 +293,36 @@ export default async function run(
);
}

if (stdin.isTTY && stdin instanceof tty.ReadStream) {
readline.emitKeypressEvents(stdin);
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
// to not be stuck in the while loop).
Promise.resolve().then(async function() {
log.info('Press R to reload (and Ctrl-C to quit)');

let userExit = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

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...
  });


while (!userExit) {
const keyPressed = await new Promise((resolve) => {
stdin.once('keypress', (str, key) => resolve(key));
});

if (keyPressed.ctrl && keyPressed.name === 'c') {
userExit = true;
} else if (keyPressed.name === 'r' && addonId) {
await addonReload({addonId, client});
}
}

log.info('\nExiting web-ext on user request');
runningFirefox.kill();
stdin.pause();
});
}

log.info('The extension will reload if any source file changes');
reloadStrategy({
firefoxProcess: runningFirefox,
Expand Down
1 change: 1 addition & 0 deletions src/firefox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ export type FirefoxRunnerParams = {|
export interface FirefoxProcess extends events$EventEmitter {
stderr: events$EventEmitter;
stdout: events$EventEmitter;
kill: Function;
}

export type FirefoxRunnerResults = {|
Expand Down
11 changes: 9 additions & 2 deletions tests/unit/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,9 @@ export function makeSureItFails(): Function {
*
*/
// $FLOW_IGNORE: fake can return any kind of object and fake a defined set of methods for testing.
export function fake<T>(original: Object, methods: Object = {}): T {
export function fake<T>(
original: Object, methods: Object = {}, skipProperties: Array<string> = []
): T {
var stub = {};

// Provide stubs for all original members:
Expand All @@ -162,7 +164,8 @@ export function fake<T>(original: Object, methods: Object = {}): T {

var proto = Object.getPrototypeOf(original);
for (const key of props) {
if (!original.hasOwnProperty(key) && !proto.hasOwnProperty(key)) {
if (skipProperties.indexOf(key) >= 0 ||
(!original.hasOwnProperty(key) && !proto.hasOwnProperty(key))) {
continue;
}
const definition = original[key] || proto[key];
Expand Down Expand Up @@ -190,6 +193,10 @@ export function fake<T>(original: Object, methods: Object = {}): T {
return stub;
}

export function createFakeProcess() {
return fake(process, {}, ['EventEmitter', 'stdin']);
}


/*
* Returns a fake Firefox client as would be returned by
Expand Down
Loading