[cli] Replace update-notifier dependency with build in #9098
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
cb1kenobi
requested review from
TooTallNate,
EndangeredMassa,
styfle and
Ethan-Arrowood
as code owners
| output: Output | undefined | ||
| ) { | ||
| // we need to find the update worker script since the location is | ||
| // different based on production vs tests |
There was a problem hiding this comment.
This part is concerning. We should be testing the actual built output
There was a problem hiding this comment.
It does test the actual build output. When running the test, __filename resolves packages/cli/src/util/get-latest-version/index.ts and when running from production, __filename resolves to dist/index.js. Then it will walk up the directory tree until it hits root (shouldn't happen) or finds dist/get-latest-worker.js.
I agree this is gross, but I seem to recall there was an issue with attempting to find get-latest-worker.js relative to get-latest-version/index.ts.
kodiakhq
bot
removed
the
pr: automerge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
| accept: | ||
| 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*', | ||
| }; | ||
| const url = `https://registry.npmjs.org/${name}`; |
There was a problem hiding this comment.
I realize now that this returns 2MB of json. Since it only runs once a week thats probably okay but that is not really nice for anyone on low data or if we later decide to check more often.
| if ( | ||
| !pkg || | ||
| typeof pkg !== 'object' || | ||
| !pkg.name || | ||
| typeof pkg.name !== 'string' | ||
| ) { | ||
| throw new TypeError('Expected package to be an object with a package name'); | ||
| } |
There was a problem hiding this comment.
Is this not handled by TypeScript?
There was a problem hiding this comment.
This was based on one of my first PRs and I have the habit of validating arguments despite living in a TypeScript world.
kodiakhq
bot
deleted the
chrisbarber/vccli-222-replace-update-notifier-with-built-in
branch
This PR replaces the
update-notifierdependency with a custom implementation.There are a few reasons: the dependency is quite large, it requires ESM in order to update, can sometimes suggest an update to an older version, and used dependencies with known security issues.
The result looks like:
Note: This PR is the successor to #8090.