feat: add upgrade reminder #3569
Conversation
|
Hi @erickzhao |
|
I see that some tests are failing. I'll look into it and let you know. |
packages/api/cli/package.json
Outdated
| "chai": "^4.3.3", | ||
| "chai-as-promised": "^7.0.0", | ||
| "mocha": "^9.0.1" | ||
| "mocha": "^9.0.1", | ||
| "update-notifier": "5.1.0" |
There was a problem hiding this comment.
issue: I think this should be under dependencies since it ships to users.
There was a problem hiding this comment.
also, can we not dynamically import the ESM module into CommonJS? Would be nice to have v6. v7 seems out of the question for now because we still support Node 16 in Forge.
await import('update-notifier')
There was a problem hiding this comment.
I tried to use the dynamic import() for v6 but kept getting the same ESM errors.
|
|
||
| import './util/terminate'; | ||
| import workingDir from './util/working-dir'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const metadata = require('../package.json'); |
There was a problem hiding this comment.
issue: This can be converted to an import statement to avoid the ESLint error.
| }, | ||
| updateCheckInterval: 1000 * 60 * 60, | ||
| }); | ||
| if (notifier.update && semver.lt(notifier.update.current, notifier.update.latest)) { |
There was a problem hiding this comment.
question: In what case is there going to be an update available and current < latest? I thought that updateNotifier would not actually report a notifier.update if there was not an update available.
There was a problem hiding this comment.
you are right
Returns an instance with an .update property if there is an available update, otherwise undefined.
I was not thorough enough with the documentation and hence used semver to double check the condition. I'm sorry :)
| updateCheckInterval: 1000 * 60 * 60, | ||
| }); | ||
| if (notifier.update && semver.lt(notifier.update.current, notifier.update.latest)) { | ||
| console.log(`Update available: ${chalk.dim(`${notifier.update?.current}`)} -> ${chalk.green(`${notifier.update?.latest}`)}`); |
There was a problem hiding this comment.
issue(blocking): I don't think this is exactly what we want here.
I think a custom log message is necessary for our monorepo design as we want to provide users with a way to update all their packages at a glance, but it seems like the custom message here simply logs the new version number without additional context?
There was a problem hiding this comment.
Additionally, I'd like to manually generate the little box that updatenotifier uses by default :)
| (async () => { | ||
| let commandArgs = process.argv; | ||
| let appArgs; | ||
| const notifier = updateNotifier({ | ||
| pkg: { |
There was a problem hiding this comment.
nit: we should just pass the package.json directly here?
|
The tests are failing because I used |
Summarize your changes:
fixes #3091
uses
update-notifierto remind user about new version of Forge.