Allow opn & update-notifier CLI deps to be optional. #2150
Conversation
lighthouse-cli/shim-modules.ts
Outdated
| @@ -0,0 +1,50 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2016 Google Inc. All rights reserved. | |||
lighthouse-cli/shim-modules.ts
Outdated
| export let opn = () => { | ||
| function shim(str: String, obj: Object) { | ||
| console.error('module `opn` not installed. Not opening browser.'); | ||
| return [str, obj] |
lighthouse-cli/shim-modules.ts
Outdated
| export let updateNotifier = () => { | ||
| function shim(obj: Object) { | ||
| console.error('module `update-notifier` not installed. Not checking for new version.'); | ||
| return { notify: () => obj }; |
lighthouse-cli/bin.ts
Outdated
| const pkg = require('../package.json'); | ||
|
|
||
| // accept noop modules for these, so the real dependency is optional. | ||
| const opn = require('./shim-modules').opn(); |
There was a problem hiding this comment.
should we do this or just try/catch their assignment and guard their usage with an if (opn) and if (updateNotifier)? They're used so rarely it seems easy to do the second option
There was a problem hiding this comment.
I was thinking the same, but we may have more of these in the future. It might be nice to keep our codebase clean rather than littering if with a bunch of forks.
There was a problem hiding this comment.
it's just that for these two it's literally just
if (updateNotifier) {
updateNotifier({pkg}).notify();
}and
if (flags.view && opn) {
opn(outputPath, {wait: false});
}the new thing has logged warnings, which is nice, but it is a lot more code to get that same effect :)
There was a problem hiding this comment.
the try/catch in the same file is really nasty and i really preferred moving it out into shim-modules.
so i think it's worth keeping that.
However, i'd be fine with guarding the calls so we don't need to mock their API.
There was a problem hiding this comment.
i'd be fine with guarding the calls so we don't need to mock their API.
done
There was a problem hiding this comment.
i'd be fine with guarding the calls so we don't need to mock their API.
done
undone. i think its worth the overhead to make sure silent failures aren't silent.
lighthouse-cli/bin.ts
Outdated
| const pkg = require('../package.json'); | ||
|
|
||
| // accept noop modules for these, so the real dependency is optional. | ||
| const opn = require('./shim-modules').opn(); |
There was a problem hiding this comment.
oooooh not literal optionalDependencies just handle them optionally :) I gotcha now
is there any benefit to having these be individual functions vs just inlining the exports? feels a little weird as functions
lighthouse-cli/bin.ts
Outdated
| const pkg = require('../package.json'); | ||
|
|
||
| // accept noop modules for these, so the real dependency is optional. | ||
| const opn = require('./shim-modules').opn; |
There was a problem hiding this comment.
nit: really up our es imports cred and do import {opn, updateNotifier} from './shim-modules'?
| }; | ||
| } | ||
|
|
||
| export { |
No description provided.