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

[cli] Replace update-notifier dependency with built in #8090

Merged
merged 78 commits into from
Dec 1, 2022
Merged
Show file tree
Hide file tree
Changes from 44 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
c38e118
[cli] Replace `update-notifier` dependency with build in
styfle Jul 6, 2022
f8b06e0
Merge branch 'main' into replace-update-notifier-with-built-in
styfle Jul 26, 2022
01fd56d
Fix spaces
styfle Jul 26, 2022
d36c1ec
added update worker to background fetch latest version
Jul 27, 2022
03d0fdb
Merge branch 'main' into replace-update-notifier-with-built-in
cb1kenobi Jul 27, 2022
81be569
code cleanup
Jul 28, 2022
c46800d
style tweak
Jul 28, 2022
c87c22a
removed bold
Jul 28, 2022
0d72cb2
add tests, code cleanup
Jul 29, 2022
21494cb
resolved merge conflicts
Jul 29, 2022
d602459
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 3, 2022
9d5660c
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 3, 2022
6a48252
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 3, 2022
11c4129
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 5, 2022
283d91d
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 9, 2022
9981b47
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 9, 2022
cbba870
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 10, 2022
eb3713e
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 15, 2022
c10679d
Merge branch 'main' into replace-update-notifier-with-built-in
EndangeredMassa Aug 17, 2022
7edf3b4
Merge branch 'main' into replace-update-notifier-with-built-in
EndangeredMassa Aug 23, 2022
b8cbb20
Update package.json
Aug 23, 2022
469cf96
Added logging for errors, fixed blocking, fixed worker script
Aug 25, 2022
0597798
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 25, 2022
e952a1f
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 25, 2022
5b7fce5
added timers to terminate long running worker process, added comments
Aug 25, 2022
21957af
Merge branch 'replace-update-notifier-with-built-in' of github.com:ve…
Aug 25, 2022
055b908
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 25, 2022
5a021cb
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 25, 2022
a652572
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 25, 2022
250c83d
added logging, fixed comment, renamed timer var
Aug 26, 2022
87c6399
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 26, 2022
1225467
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 26, 2022
035aea4
removed worker timeout log message
Aug 26, 2022
9ef6d5d
Merge branch 'main' into replace-update-notifier-with-built-in
Aug 29, 2022
c2ad59b
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 7, 2022
78a79e1
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 13, 2022
72835e8
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 14, 2022
118cd7b
Refactored the API to be completely async
Sep 15, 2022
d7a122c
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 15, 2022
2735cb5
Create update notifier dir and improved tests
Sep 16, 2022
3bb5005
Tweaks and improved comments
Sep 16, 2022
1633be7
Cleaned up naming and improved comments
Sep 16, 2022
d1c15d5
Added logging to the get latest update worker, force worker to exit w…
Sep 16, 2022
e46eadb
Added comments, fixed exit code, call finally if already running
Sep 19, 2022
da2d962
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 20, 2022
7dbeda7
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 21, 2022
21b5106
Renamed getLatestInterval to updateCheckInterval and explicitly pass …
Sep 21, 2022
fb9ae3c
Removed defaultUpdateCheckInterval and added simple pkg validation
Sep 21, 2022
14edff0
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 21, 2022
4cd39b0
Removed updateCheckInterval in favor of default
Sep 21, 2022
abc40e3
Merge branch 'main' into replace-update-notifier-with-built-in
kodiakhq[bot] Sep 21, 2022
8caddf5
Merge branch 'main' into replace-update-notifier-with-built-in
kodiakhq[bot] Sep 21, 2022
d23ef97
Merge branch 'main' into replace-update-notifier-with-built-in
kodiakhq[bot] Sep 21, 2022
194d888
Merge branch 'main' into replace-update-notifier-with-built-in
kodiakhq[bot] Sep 21, 2022
cd50dfa
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 22, 2022
a30df0e
Merge branch 'main' into replace-update-notifier-with-built-in
kodiakhq[bot] Sep 22, 2022
975d0a7
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 22, 2022
35f0f8c
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 29, 2022
53490b7
Merge branch 'main' into replace-update-notifier-with-built-in
Sep 30, 2022
8f8508d
Merge branch 'main' into replace-update-notifier-with-built-in
Oct 2, 2022
d41e739
Merge branch 'main' into replace-update-notifier-with-built-in
Oct 5, 2022
4287e2d
Merge branch 'main' into replace-update-notifier-with-built-in
Oct 5, 2022
99c82c0
Merge branch 'main' into replace-update-notifier-with-built-in
Oct 5, 2022
286dd6a
Merge branch 'main' into replace-update-notifier-with-built-in
Oct 5, 2022
6d45a3f
Merge branch 'main' into replace-update-notifier-with-built-in
Oct 6, 2022
c91e5da
Merge branch 'main' into replace-update-notifier-with-built-in
Oct 6, 2022
1a768cc
Merge branch 'main' into replace-update-notifier-with-built-in
Oct 11, 2022
2a3540f
Merge branch 'main' into replace-update-notifier-with-built-in
Oct 11, 2022
de4b687
Merge branch 'main' into replace-update-notifier-with-built-in
styfle Oct 14, 2022
205b6c8
Merge branch 'main' into replace-update-notifier-with-built-in
Nov 21, 2022
a3cb3b0
Merge branch 'main' into replace-update-notifier-with-built-in
Nov 23, 2022
c08c50c
Merge branch 'main' into replace-update-notifier-with-built-in
Nov 23, 2022
7d040cb
Merge branch 'main' into replace-update-notifier-with-built-in
Nov 28, 2022
def2926
Merge branch 'main' into replace-update-notifier-with-built-in
Nov 29, 2022
226a232
Merge branch 'main' into replace-update-notifier-with-built-in
Nov 30, 2022
5523b07
Merge branch 'main' into replace-update-notifier-with-built-in
Nov 30, 2022
c765778
Merge branch 'main' into replace-update-notifier-with-built-in
Dec 1, 2022
e1297f7
Merge branch 'main' into replace-update-notifier-with-built-in
Dec 1, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions packages/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
},
"scripts": {
"preinstall": "node ./scripts/preinstall.js",
"test": "jest --env node --verbose --runInBand --bail --forceExit",
"test": "jest --env node --verbose --runInBand --bail",
Copy link
Contributor

Choose a reason for hiding this comment

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

Removed --forceExit because the update notifier is async and the process will out live the test and causes jest to complain.

"test-unit": "yarn test test/unit/",
"test-integration-cli": "rimraf test/fixtures/integration && ava test/integration.js --serial --fail-fast --verbose",
"test-integration-dev": "yarn test test/dev/",
Expand Down Expand Up @@ -50,8 +50,7 @@
"@vercel/redwood": "1.0.23",
"@vercel/remix": "1.0.24",
"@vercel/ruby": "1.3.31",
"@vercel/static-build": "1.0.23",
"update-notifier": "5.1.0"
"@vercel/static-build": "1.0.23"
},
"devDependencies": {
"@alex_neo/jest-expect-message": "1.0.5",
Expand Down
22 changes: 13 additions & 9 deletions packages/cli/scripts/build.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import cpy from 'cpy';
import execa from 'execa';
import { join } from 'path';
import { remove, writeFile } from 'fs-extra';
import { remove, readJSON, writeFile } from 'fs-extra';

const dirRoot = join(__dirname, '..');
const distRoot = join(dirRoot, 'dist');
Expand Down Expand Up @@ -43,15 +43,15 @@ async function main() {
stdio: 'inherit',
});

const pkg = await readJSON(join(dirRoot, 'package.json'));
const dependencies = Object.keys(pkg?.dependencies ?? {});
cb1kenobi marked this conversation as resolved.
Show resolved Hide resolved
// Do the initial `ncc` build
console.log();
const args = [
'ncc',
'build',
'--external',
'update-notifier',
'src/index.ts',
];
console.log('Dependencies:', dependencies);
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking - what if dependencies is empty? Should we still log?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update this in a follow-up PR?

We probably don't want to log if dependencies is empty. Or, at least we'd want to log a clearer message saying so.

Copy link
Contributor

Choose a reason for hiding this comment

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

On one hand, this is a build script and if there are no dependencies, then I'd think we'd want to see that.

On the other hand, dependencies will likely never be empty.

const externs = [];
for (const dep of dependencies) {
externs.push('--external', dep);
}
const args = ['ncc', 'build', 'src/index.ts', ...externs];
await execa('yarn', args, { stdio: 'inherit', cwd: dirRoot });

// `ncc` has some issues with `@vercel/fun`'s runtime files:
Expand All @@ -78,6 +78,10 @@ async function main() {
// Band-aid to bundle stuff that `ncc` neglects to bundle
await cpy(join(dirRoot, 'src/util/projects/VERCEL_DIR_README.txt'), distRoot);
await cpy(join(dirRoot, 'src/util/dev/builder-worker.js'), distRoot);
await cpy(
join(dirRoot, 'src/util/get-latest-version/get-latest-worker.js'),
distRoot
);

console.log('Finished building Vercel CLI');
}
Expand Down
47 changes: 24 additions & 23 deletions packages/cli/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import sourceMap from '@zeit/source-map-support';
import { mkdirp } from 'fs-extra';
import chalk from 'chalk';
import epipebomb from 'epipebomb';
import updateNotifier from 'update-notifier';
import getLatestVersion from './util/get-latest-version';
import { URL } from 'url';
import * as Sentry from '@sentry/node';
import hp from './util/humanize-path';
Expand Down Expand Up @@ -55,13 +55,6 @@ import { VercelConfig } from '@vercel/client';

const isCanary = pkg.version.includes('canary');

// Checks for available update and returns an instance
const notifier = updateNotifier({
pkg,
distTag: isCanary ? 'canary' : 'latest',
updateCheckInterval: 1000 * 60 * 60 * 24 * 7, // 1 week
});

const VERCEL_DIR = getGlobalPathConfig();
const VERCEL_CONFIG_PATH = configFiles.getConfigFilePath();
const VERCEL_AUTH_CONFIG_PATH = configFiles.getAuthConfigFilePath();
Expand Down Expand Up @@ -149,22 +142,30 @@ const main = async () => {
}

// Print update information, if available
if (notifier.update && notifier.update.latest !== pkg.version && isTTY) {
const { latest } = notifier.update;
console.log(
info(
`${chalk.black.bgCyan('UPDATE AVAILABLE')} ` +
`Run ${cmd(
await getUpdateCommand()
)} to install ${getTitleName()} CLI ${latest}`
)
);
if (isTTY && !process.env.NO_UPDATE_NOTIFIER) {
// Check if an update is available. If so, `latest` will contain a string
// of the latest version, otherwise `undefined`.
const latest = getLatestVersion({
distTag: isCanary ? 'canary' : 'latest',
output,
pkg,
styfle marked this conversation as resolved.
Show resolved Hide resolved
});
cb1kenobi marked this conversation as resolved.
Show resolved Hide resolved
if (latest) {
console.log(
info(
`${chalk.black.bgCyan('UPDATE AVAILABLE')} ` +
`Run ${cmd(
await getUpdateCommand()
)} to install ${getTitleName()} CLI ${latest}`
)
);

console.log(
info(
`Changelog: https://github.com/vercel/vercel/releases/tag/vercel@${latest}`
)
);
console.log(
Copy link
Member

Choose a reason for hiding this comment

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

We should be using output.log() here instead of console.log() otherwise we'll break the pipe-ability of commands (i.e. deployment URL goes to stdout for vc deploy).

I realize that this was the previous behavior as well, but since we're here we might as well fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the isTTY check, this won't break piped output, but by convention and for future testing, we should still probably change these to use output in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update this in a follow-up PR?

This was about using output instead of console.log.

`${info(
`Changelog: https://github.com/vercel/vercel/releases/tag/vercel@${latest}`
)}\n`
);
}
}

// The second argument to the command can be:
Expand Down
188 changes: 188 additions & 0 deletions packages/cli/src/util/get-latest-version/get-latest-worker.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
/**
* This file is spawned in the background and checks npm for the latest version
* of the CLI, then writes the version to the cache file.
*
* NOTE: Since this file runs asynchronously in the background, it's possible
* for multiple instances of this file to be running at the same time leading
* to a race condition where the most recent instance will overwrite the
* previous cache file resetting the `notified` flag and cause the update
* notification to appear for multiple consequetive commands. Not the end of
* the world, but something to be aware of.
*/

const fetch = require('node-fetch');
const fs = require('fs-extra');
const path = require('path');
const { Agent: HttpsAgent } = require('https');
const { bold, gray, red } = require('chalk');
const { format, inspect } = require('util');

/**
* An simple output helper which accumulates error and debug log messages in
* memory for potential persistance to disk while immediately outputting errors
* and debug messages, when the `--debug` flag is set, to `stderr`.
*/
class WorkerOutput {
debugLog = [];
logFile = null;

constructor({ debug = true }) {
this.debugOutputEnabled = debug;
}

debug(...args) {
this.print('debug', args);
}

error(...args) {
this.print('error', args);
}

print(type, args) {
const str = format(
...args.map(s => (typeof s === 'string' ? s : inspect(s)))
);
this.debugLog.push(`[${new Date().toISOString()}] [${type}] ${str}`);
if (type === 'debug' && this.debugOutputEnabled) {
console.error(
`${gray('>')} ${bold('[debug]')} ${gray(
`[${new Date().toISOString()}]`
)} ${str}`
);
} else if (type === 'error') {
console.error(`${red(`Error:`)} ${str}`);
}
}

setLogFile(file) {
// wire up the exit handler the first time the log file is set
if (this.logFile === null) {
process.on('exit', () => {
EndangeredMassa marked this conversation as resolved.
Show resolved Hide resolved
if (this.debugLog.length) {
fs.outputFileSync(this.logFile, this.debugLog.join('\n'));
}
});
}
this.logFile = file;
}
}

const output = new WorkerOutput({
// enable the debug logging if the `--debug` is set or if this worker script
// was directly executed
debug: process.argv.includes('--debug') || !process.connected,
EndangeredMassa marked this conversation as resolved.
Show resolved Hide resolved
});

process.on('unhandledRejection', err => {
output.error('Exiting worker due to unhandled rejection:', err);
process.exit(1);
});

const defaultGetLatestInterval = 1000 * 60 * 60 * 24 * 7; // 1 week

// this timer will prevent this worker process from running longer than 10s
const timer = setTimeout(() => {
output.error('Worker timed out after 10 seconds');
process.exit(1);
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this needs to clean up the lock file. Otherwise the user will never be able to update again.

I'm wondering if we even need a lock file at all 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

This setTimeout() exists in the event the parent never sends the message, thus no lock file will be created to be cleaned up. The lock file name is derived from the cache file name, which is the package name plus the dist tag. Those two bits of info are not available at the time the timer is created.

We could rename timer to waitForParentTimer, then clear it once it gets the message to start, then create an AbortController to make sure the fetch() returns in a reasonable time.

Lock files are only 4KB on disk and I have yet to come across a scenario where the lock file wasn't cleaned up. I'm sure it will happen at some point and that's why I added the stale pid check in isRunning().

As for needing the lock file, I did some testing and it's possible to run vercel commands fast enough that one worker is clobbering the cache file with stale data (e.g. the notified flag being toggled). Since the access to the cache file should be atomic, a lock file will synchronize multiple processes ensuring a predictable state. Obviously we're not building a database, so if you feel the lock file, or the whole IPC design for that matter, is overkill, I'd be happy to refactor the code.

}, 10000);

// wait for the parent to give us the work payload
process.once('message', async msg => {
output.debug('Received message from parent:', msg);

output.debug('Disconnecting from parent');
process.disconnect();

const { cacheFile, distTag, name, getLatestInterval } = msg;
const cacheFileParsed = path.parse(cacheFile);
await fs.mkdirp(cacheFileParsed.dir);

output.setLogFile(
path.join(cacheFileParsed.dir, `${cacheFileParsed.name}.log`)
);

const lockFile = path.join(
cacheFileParsed.dir,
`${cacheFileParsed.name}.lock`
);

try {
// check for a lock file and either bail if running or write our pid and continue
output.debug(`Checking lock file: ${lockFile}`);
if (await isRunning(lockFile)) {
output.debug('Worker already running, exiting');
process.exit(1);
}
output.debug(`Initializing lock file with pid ${process.pid}`);
await fs.writeFile(lockFile, String(process.pid), 'utf-8');

// fetch the latest version from npm
const agent = new HttpsAgent({
keepAlive: true,
maxSockets: 15, // See: `npm config get maxsockets`
});
const headers = {
accept:
'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*',
};
const url = `https://registry.npmjs.org/${name}`;
output.debug(`Fetching ${url}`);
const res = await fetch(url, { agent, headers });
const json = await res.json();
const tags = json['dist-tags'];
const version = tags[distTag];

if (version) {
output.debug(`Found dist tag "${distTag}" with version "${version}"`);
} else {
output.error(`Dist tag "${distTag}" not found`);
output.debug('Available dist tags:', Object.keys(tags));
}

output.debug(`Writing cache file: ${cacheFile}`);
await fs.outputJSON(cacheFile, {
expireAt: Date.now() + (getLatestInterval || defaultGetLatestInterval),
styfle marked this conversation as resolved.
Show resolved Hide resolved
notified: false,
version,
});
} catch (err) {
output.error(`Failed to get package info:`, err);
} finally {
clearTimeout(timer);

output.debug(`Releasing lock file: ${lockFile}`);
await fs.remove(lockFile);

output.debug(`Worker finished successfully!`);

// force the worker to exit
process.exit(0);
}
});

// signal the parent process we're ready
if (process.connected) {
output.debug("Notifying parent we're ready");
process.send({ type: 'ready' });
} else {
console.error('No IPC bridge detected, exiting');
process.exit(1);
}

async function isRunning(lockFile) {
try {
const pid = parseInt(await fs.readFile(lockFile, 'utf-8'));
output.debug(`Found lock file with pid: ${pid}`);

// checks for existence of a process; throws if not found
process.kill(pid, 0);

// process is still running
return true;
} catch (err) {
// lock file does not exist or process is not running and pid is stale
output.debug(`Resetting lock file: ${err.toString()}`);
await fs.remove(lockFile);
return false;
}
}