feat: web-ext automatically checks for updates #676
Conversation
| "node-firefox-connect": "1.2.0", | ||
| "regenerator-runtime": "0.10.1", | ||
| "parse-json": "2.2.0", | ||
| "regenerator-runtime": "0.10.0", |
There was a problem hiding this comment.
this line can be removed (regenerator-runtime is already in the dependencies at line 60)
| version: defaultVersionGetter(absolutePackageDir), | ||
| }; | ||
|
|
||
| updateNotifier({ |
There was a problem hiding this comment.
we should probably move this before the await runCommand(argv); that is at line 112 (so that we check the version before running the actual command)
| } | ||
| await runCommand(argv); | ||
|
|
||
| let pkg = { |
There was a problem hiding this comment.
pkg is never re-assigned again, and so we can use const instead of let
kumar303
left a comment
There was a problem hiding this comment.
Breaking it into a utility function is definitely the way to go. I added some pointers about how to get started on the tests. Let me know if you have questions.
| @@ -0,0 +1,25 @@ | |||
| /* @flow */ | |||
| import updateNotifier from 'update-notifier'; | |||
There was a problem hiding this comment.
The key to testing this function is to stub out the updateNotifier since we can trust it works as documented. The goal of the tests will be to make sure that web-ext is configuring the notifier correctly.
First, import the original package as a default that can be customized:
import defaultUpdateNotifier from 'update-notifier';Next, use dependency injection so that it can be replaced:
export function checkForAutomaticUpdates(
{
name,
version,
updateCheckInterval,
updateNotifier = defaultUpdateNotifier,
}: checkForAutomaticUpdatesParams
) {
// ...
updateNotifier();
}This allows you to test it with a stub:
const updateNotifier = sinon.stub();
checkForAutomaticUpdates({name, version, updateNotifier});
assert.equal(updateNotifier.firstCall.args[0].pkg.name, 'web-ext');| argv: ?Array<string>, | ||
| {absolutePackageDir = process.cwd()}: {absolutePackageDir?: string} = {} | ||
| { | ||
| absolutePackageDir = process.cwd(), |
There was a problem hiding this comment.
similarly, this module / class should be set up for dependency injection so that you can replace the update utility with a stub for testing:
import {checkForAutomaticUpdates as defaultUpdateChecker} from './util/updates';
class Program {
constructor(
argv: ?Array<string>,
{
absolutePackageDir?: string,
checkForAutomaticUpdates = defaultUpdateChecker,
}
) {
// ...
this.checkForUpdates = checkForAutomaticUpdates;
}
}The test can then use a stub:
const checkForAutomaticUpdates = sinon.stub();
const program = new Program(['run'], {checkForAutomaticUpdates});
await program.execute();
assert.equal(checkForAutomaticUpdates.firstCall.args[0].version, '1.0.0');|
|
||
| this.checkForUpdates ({ | ||
| name: 'web-ext', | ||
| version: defaultVersionGetter(absolutePackageDir), |
There was a problem hiding this comment.
this function already has a getVersion parameter which can be replaced by a stub for testing. You can use it like checkForUpdates({version: getVersion(absolutePackageDir)})
1 similar comment
kumar303
left a comment
There was a problem hiding this comment.
Looking good! I requested some cleanup.
I think we also need a --disable-update-check option but I suggest filing that as a separate issue and doing it in a different patch.
| updateNotifier: typeof defaultUpdateNotifier, | ||
| }; | ||
|
|
||
| export function checkForAutomaticUpdates({ |
There was a problem hiding this comment.
The update would technically need to be applied manually, not automatically, so I think checkForUpdates() would be a better name.
| yargs: any; | ||
| commands: { [key: string]: Function }; | ||
| shouldExitProgram: boolean; | ||
| checkForUpdates: typeof defaultUpdateChecker; |
There was a problem hiding this comment.
Looks like you removed this class var so it no longer needs to be declared
|
|
||
| this.shouldExitProgram = shouldExitProgram; | ||
| this.yargs.exitProcess(this.shouldExitProgram); | ||
| this.checkForUpdates = checkForUpdates; |
There was a problem hiding this comment.
it's no longer necessary to assign the function to this because it's not used anywhere else in the class
| }: checkForAutomaticUpdatesParams | ||
| ) { | ||
| const pkg = { | ||
| name: name, |
There was a problem hiding this comment.
Why does name need to be configurable? I think you can just hard code it as name: 'web-ext'.
| ) { | ||
| const pkg = { | ||
| name: name, | ||
| version: version, |
There was a problem hiding this comment.
Always use shorthand property assignment when possible, like: {version}
There was a problem hiding this comment.
We should probably add the lint rule for this :D http://eslint.org/docs/rules/object-shorthand
| import { | ||
| defaultVersionGetter, | ||
| main, | ||
| Program, |
There was a problem hiding this comment.
Any reason why you split these into multiple lines? Since the previous statement was less than 80 characters, you don't need to split it.
There was a problem hiding this comment.
I added another import, so ESLint started yelling, hence I split them up but ended up removing that import and forgot to put them back in one line.
| .command('thing', 'does a thing', thing); | ||
| return execProgram(program) | ||
| return execProgram(program, { | ||
| checkForUpdates: spy(), |
There was a problem hiding this comment.
Passing this directly creates unnecessary repetition. I don't see what the problem is with adding it as a default to execProgram(). What problem were you running into?
There was a problem hiding this comment.
It wasn't being passed by default in execProgram() and making some of the tests fail. Not sure why.
There was a problem hiding this comment.
Maybe it wasn't defined in the right part of execProgram()? If it's not working, post a broken patch and I can take a look.
| const handler = spy(); | ||
| const checkForAutomaticUpdates = sinon.stub(); | ||
| const program = new Program(['run']) | ||
| .command('run', 'some command', handler); |
There was a problem hiding this comment.
This should be indented two spaces to show that it's a continuation of the call up above.
| 'web-ext'); | ||
| let pkgFile = path.join(root, 'package.json'); | ||
| return fs.readFile(pkgFile) | ||
| .then((pkgData) => { |
There was a problem hiding this comment.
You should always chain promises instead of nest them. Otherwise you might as well use callbacks :) Chaining would look like:
return execProgram()
.then(() => {
assert.equal(...);
return fs.readFile(pkgFile);
})
.then((pkgFile) => {
assert.equal(...);
});| return fs.readFile(pkgFile) | ||
| .then((pkgData) => { | ||
| assert.equal(checkForAutomaticUpdates.firstCall.args[0].version, | ||
| JSON.parse(pkgData).version); |
There was a problem hiding this comment.
I like this test! Good idea.
kumar303
left a comment
There was a problem hiding this comment.
I mostly requested nits so this is really close!
|
|
||
| checkForUpdates ({ | ||
| version: getVersion(absolutePackageDir), | ||
| updateNotifier: defaultUpdateChecker, |
There was a problem hiding this comment.
why does updateNotifier need to be passed? What does it do? I'm confused.
| { | ||
| absolutePackageDir = process.cwd(), | ||
| }: { | ||
| absolutePackageDir?: string, |
There was a problem hiding this comment.
even though there is only one param, I think it's worth moving this to a standalone type object like we do elsewhere:
type ProgramOptions = {
absolutePackageDir?: string,
}| export function checkForUpdates({ | ||
| version, | ||
| updateNotifier = defaultUpdateNotifier, | ||
| }: checkForUpdatesParams |
There was a problem hiding this comment.
nit: I think it would be easier to visualize the nesting of these statements if they were indented one more level like:
export function checkForUpdates(
{
version,
updateNotifier = defaultUpdateNotifier,
}: checkForUpdatesParams
) {
...
}| describe('util/updates', () => { | ||
| describe('checkForUpdates()', () => { | ||
| it('calls the notifier with the correct parameters', () => { | ||
| let updateNotifierStub = sinon.spy(() => { |
| type checkForUpdatesParams = { | ||
| version: string, | ||
| updateNotifier?: typeof defaultUpdateNotifier, | ||
| }; |
There was a problem hiding this comment.
This should use the new pipe syntax in Flow that says "only these parameters are allowed" :
type checkForUpdatesParams = {|
version: string,
updateNotifier?: typeof defaultUpdateNotifier,
|};There was a problem hiding this comment.
gah, it's not documented yet (or I just can't find where)
| }); | ||
|
|
||
| checkForUpdates({ | ||
| name: 'web-ext', |
There was a problem hiding this comment.
this call passes a bunch of unnecessary parameters but once you update the Flow type you should get errors about them
| }) | ||
| .then(() => { | ||
| assert.equal(checkForAutomaticUpdates.firstCall.args[0].name, | ||
| 'web-ext'); |
There was a problem hiding this comment.
nit: this should line up with the opening parenthesis
There was a problem hiding this comment.
actually, you don't need to make this assertion because program is no longer responsible for passing the name of the package
| }) | ||
| .then((pkgData) => { | ||
| assert.equal(checkForAutomaticUpdates.firstCall.args[0].version, | ||
| JSON.parse(pkgData).version); |
There was a problem hiding this comment.
nit: this should line up with the opening parenthesis of the assert.equal() call
kumar303
left a comment
There was a problem hiding this comment.
This time I gave it a test run. Looks great! I found some things that need changing.
| { | ||
| version, | ||
| updateNotifier = defaultUpdateNotifier, | ||
| }: checkForUpdatesParams |
| }) | ||
| .then(() => { | ||
| assert.equal(checkForAutomaticUpdates.firstCall.args[0].version, | ||
| defaultVersionGetter(root)); |
There was a problem hiding this comment.
nit: this is indented too much. It should line up with the opening paren like:
assert.equal(checkForAutomaticUpdates.firstCall.args[0].version,
defaultVersionGetter(root));| assert.equal(updateNotifierStub.firstCall.args[0].pkg.name, 'web-ext'); | ||
| assert.equal(updateNotifierStub.firstCall.args[0].pkg.version, '1.0.0'); | ||
| assert.equal(updateNotifierStub.firstCall.args[0].updateCheckInterval, | ||
| 1000 * 60 * 60 * 24 * 7); |
There was a problem hiding this comment.
nit: indent should line up with opening paren of the call
| it('checks for updates automatically', () => { | ||
| const root = path.join(__dirname, '..', '..'); | ||
| const handler = spy(); | ||
| const checkForAutomaticUpdates = sinon.stub(); |
There was a problem hiding this comment.
I know this name is left over from before; I think you should change it to avoid confusion. Actually, if you call it checkForUpdates then you'd be able to use the object property shorthand below.
| "node-firefox-connect": "1.2.0", | ||
| "regenerator-runtime": "0.10.1", | ||
| "parse-json": "2.2.0", | ||
| "regenerator-runtime": "0.10.0", |
There was a problem hiding this comment.
oops, I almost missed this. Please be careful when resolving conflicts. The version number was bumped down in your patch.
There was a problem hiding this comment.
I didn't touch package.json at all while resolving the conflicts. There was no conflict in this file that needed to be resolved manually. I get git did it on its own?
|
|
||
| type ProgramOptions = { | ||
| absolutePackageDir?: string, | ||
| } |
There was a problem hiding this comment.
while you're already in here fixing nits how about locking down these object properties?
type ProgramOptions = {|
absolutePackageDir?: string,
|}We eventually need to do this for all object types but don't worry about locking down others that are unrelated to your patch.
| }) | ||
| .then(() => { | ||
| assert.equal(checkForAutomaticUpdates.firstCall.args[0].version, | ||
| defaultVersionGetter(root)); |
There was a problem hiding this comment.
I noticed some test failures locally (surprisingly not on Travis though). I'm going to suggest a few things.
- Stub out
getVersionfor all tests that are not covering any version logic - Use a stub
getVersionfunction in your update checker test. I'm suggesting this because the behavior ofdefaultVersionGetteris already covered in other tests.
Here is a patch to illustrate what I mean:
diff --git a/tests/unit/test.program.js b/tests/unit/test.program.js
index 58732b8..a4eea98 100644
--- a/tests/unit/test.program.js
+++ b/tests/unit/test.program.js
@@ -21,6 +21,7 @@ describe('program.Program', () => {
let absolutePackageDir = path.join(__dirname, '..', '..');
return program.execute(
absolutePackageDir, {
+ getVersion: () => 'not-a-real-version',
checkForUpdates: spy(),
systemProcess: fakeProcess,
shouldExitProgram: false,
@@ -251,12 +252,14 @@ describe('program.Program', () => {
const checkForAutomaticUpdates = sinon.stub();
const program = new Program(['run'])
.command('run', 'some command', handler);
+ const getVersion = () => 'some-package-version';
return execProgram(program, {
+ getVersion,
checkForUpdates: checkForAutomaticUpdates,
})
.then(() => {
assert.equal(checkForAutomaticUpdates.firstCall.args[0].version,
- defaultVersionGetter(root));
+ 'some-package-version');
});
});
});
@@ -266,6 +269,7 @@ describe('program.main', () => {
function execProgram(argv, {projectRoot = '', ...mainOptions}: Object = {}) {
const runOptions = {
+ getVersion: () => 'not-a-real-version',
checkForUpdates: spy(),
shouldExitProgram: false,
systemProcess: fake(process),| } | ||
| await runCommand(argv); | ||
|
|
||
| checkForUpdates ({ |
There was a problem hiding this comment.
This needs to start before the command starts, at least it does for the run command. I'm not totally sure why, I think it's because of how run exits when Firefox quits. I think it's safe to move it up because the updater runs unobtrusively in an async child process.
diff --git a/src/program.js b/src/program.js
index 1f49966..33b34da 100644
--- a/src/program.js
+++ b/src/program.js
@@ -119,12 +119,13 @@ export class Program {
if (!runCommand) {
throw new UsageError(`Unknown command: ${cmd}`);
}
- await runCommand(argv);
checkForUpdates ({
version: getVersion(absolutePackageDir),
});
+ await runCommand(argv);
+
} catch (error) {
const prefix = cmd ? `${cmd}: ` : '';
if (!(error instanceof UsageError) || argv.verbose) {It looks nice!
|
@shubheksha oh, I forgot to mention, you need to disable the update check when in development (because it will say web-ext is always out of date). Something like this: if (localEnv === 'production') {
checkForUpdates({version: getVersion(absolutePackageDir)});
}You can set up |
kumar303
left a comment
There was a problem hiding this comment.
It's getting closer. Always make sure to test your patches manually before requesting a review. For me, I do this a bit during development and then always at the final step before requesting a review from someone.
| assert.equal(updateNotifierStub.firstCall.args[0].pkg.name, 'web-ext'); | ||
| assert.equal(updateNotifierStub.firstCall.args[0].pkg.version, '1.0.0'); | ||
| assert.equal(updateNotifierStub.firstCall.args[0].updateCheckInterval, | ||
| 1000 * 60 * 60 * 24 * 7); |
There was a problem hiding this comment.
Sorry if I wasn't clear about this style rule. We always align the second argument to the inside of the open parenthesis which allows the reader to easily see that the statement within parentheses continues on the next line:
assert.equal(updateNotifierStub.firstCall.args[0].updateCheckInterval,
1000 * 60 * 60 * 24 * 7);We don't have the lint rule for this enabled because it causes some other problems.
| return execProgram(program, { | ||
| checkForUpdates, | ||
| getVersion, | ||
| localEnv: 'production', |
There was a problem hiding this comment.
This needs one more test that does localEnv: 'development' and makes sure the update checker doesn't get called, like assert.equal(checkForUpdates.called, false)
| getVersion = defaultVersionGetter, shouldExitProgram = true, | ||
| checkForUpdates = defaultUpdateChecker, systemProcess = process, | ||
| logStream = defaultLogStream, getVersion = defaultVersionGetter, | ||
| shouldExitProgram = true, localEnv = 'development', |
There was a problem hiding this comment.
This won't work in production :)
We have a magic build variable which is a bit confusing. You would use it like this:
localEnv = WEBEXT_BUILD_ENV
This will set localEnv to the value of NODE_ENV at the time that web-ext was built.
Are you testing your patch manually? Automated tests are great but you also need to test it manually. The way I was doing it was temporarily decrementing the version to 1.5.0 in package.json (just for testing), running NODE_ENV=production web-ext build to get a build, then trying web-ext run or whatever to see the update notification.
…a new file as a utility function
…ecking for self-updates
kumar303
left a comment
There was a problem hiding this comment.
I'd like to change the update interval. I requested a couple other nits too but this is really close.
|
|
||
| updateNotifier({ | ||
| pkg, | ||
| updateCheckInterval: 1000 * 60 * 60 * 24 * 7, // 1 week, |
There was a problem hiding this comment.
I think we should make this three days
| getVersion = defaultVersionGetter, shouldExitProgram = true, | ||
| checkForUpdates = defaultUpdateChecker, systemProcess = process, | ||
| logStream = defaultLogStream, getVersion = defaultVersionGetter, | ||
| shouldExitProgram = true, localEnv = WEBEXT_BUILD_ENV, |
There was a problem hiding this comment.
Hmm, localEnv doesn't really make sense. I'm not sure why we called the last one that. I would suggest naming this globalEnv because that's what it is!
| assert.equal(updateNotifierStub.firstCall.args[0].pkg.name, 'web-ext'); | ||
| assert.equal(updateNotifierStub.firstCall.args[0].pkg.version, '1.0.0'); | ||
| assert.equal(updateNotifierStub.firstCall.args[0].updateCheckInterval, | ||
| 1000 * 60 * 60 * 24 * 7); |
There was a problem hiding this comment.
I didn't mention this last review but an assertion like this isn't so helpful because when you update the interval, you have to update the test too. I would suggest a looser check, something that just makes sure the option is set and that it's a number:
assert.isNumber(updateNotifierStub.firstCall.args[0].updateCheckInterval);| const runOptions = {shouldExitProgram: false, systemProcess: fake(process)}; | ||
| const runOptions = { | ||
| getVersion: () => 'not-a-real-version', | ||
| checkForUpdates: spy(), |
There was a problem hiding this comment.
Since no assertions are made in these tests, you could just assign it as a sinon.stub() which does nothing. Doesn't really matter either way though.
|
@kumar303, could you review this again, please? |
kumar303
left a comment
There was a problem hiding this comment.
Nice one! Let's hope this cuts down on people submitting bug reports against old versions.
| //A defintion of type of argument for defaultVersionGetter | ||
| type versionGetterOptions = { | ||
| localEnv?: string, | ||
| globalEnv?: string, |
feat: web-ext automatically checks for updates (mozilla#676)

Fixes #142