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.

Sign up for GitHub

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: add upgrade reminder #3569

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: add upgrade reminder #3569

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
892c368
cli: add upgrade reminder
DevanceJ Apr 18, 2024
90edbab
makes update-notifier a dev dependency
DevanceJ Apr 18, 2024
fb4a1b2
igonores unpublished import
DevanceJ Apr 18, 2024
9a61e5b
Merge branch 'main' into feat/upgrade-reminder
DevanceJ Apr 22, 2024
4be292d
imporves update notification message and package.json handling
DevanceJ Apr 22, 2024
f97ebe0
Merge branch 'main' into feat/upgrade-reminder
DevanceJ May 29, 2024
43898e3
Merge branch 'main' into feat/upgrade-reminder
DevanceJ Jul 6, 2024
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
4 changes: 3 additions & 1 deletion packages/api/cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@
},
"devDependencies": {
"@malept/cross-spawn-promise": "^2.0.0",
"@types/update-notifier": "6.0.8",
"chai": "^4.3.3",
"chai-as-promised": "^7.0.0",
"mocha": "^9.0.1"
"mocha": "^9.0.1",
"update-notifier": "5.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

issue: I think this should be under dependencies since it ships to users.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use the dynamic import() for v6 but kept getting the same ESM errors.

},
"dependencies": {
"@electron-forge/core": "7.4.0",
Expand Down
16 changes: 16 additions & 0 deletions packages/api/cli/src/electron-forge-start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,31 @@ import path from 'path';

import { api, StartOptions } from '@electron-forge/core';
import { ElectronProcess } from '@electron-forge/shared-types';
import chalk from 'chalk';
import program from 'commander';
import fs from 'fs-extra';
import semver from 'semver';
// eslint-disable-next-line node/no-unpublished-import
import updateNotifier from 'update-notifier';

import './util/terminate';
import workingDir from './util/working-dir';

// eslint-disable-next-line @typescript-eslint/no-var-requires
const metadata = require('../package.json');
Copy link
Member

Choose a reason for hiding this comment

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

issue: This can be converted to an import statement to avoid the ESLint error.

(async () => {
let commandArgs = process.argv;
let appArgs;
const notifier = updateNotifier({
pkg: {
Copy link
Member

Choose a reason for hiding this comment

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

nit: we should just pass the package.json directly here?

name: metadata.name,
version: metadata.version,
},
updateCheckInterval: 1000 * 60 * 60,
});
if (notifier.update && semver.lt(notifier.update.current, notifier.update.latest)) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

console.log(`Update available: ${chalk.dim(`${notifier.update?.current}`)} -> ${chalk.green(`${notifier.update?.latest}`)}`);
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, I'd like to manually generate the little box that updatenotifier uses by default :)

}

const doubleDashIndex = process.argv.indexOf('--');
if (doubleDashIndex !== -1) {
Expand Down
Loading