-
Notifications
You must be signed in to change notification settings - Fork 329
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
feat: bun as packageManager option #1309
Conversation
Hi, thanks for the contribution. This looks good to me. Just need to get the tests passing. |
One file left needs some work, it's the bun package manager spawner file. I'm making this PR, but I do not yet understand the bare minimum requirement for that file. |
The minimum packageManager file just needs a function to return the latest version: {
latest: (packageName: string,
currentVersion: string) =>
{ error?: string, version?: number }
} Additional targets (patch, minor, newest, greatest) can be added by defining their respective functions, which have the same signature. You may also consider defining a The full |
@raineorshine I just implemented the basic bun spawner |
src/package-managers/bun.ts
Outdated
/** | ||
* (Bun) Fetches the list of all installed packages. | ||
*/ | ||
export const list = npmList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should spawn bun for the list command, as npm may have a different global install directory than bun.
Hopefully bun has a --json
option, otherwise you'll have to parse the results.
|
||
describe('bun', function () { | ||
it('list', async () => { | ||
const versionObject = await bun.list({ cwd: __dirname }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though there are some old tests remaining that operate on test files in the project directory tree, I prefer a dynamic setup in a temp directory as seen here:
npm-check-updates/test/bin.test.ts
Lines 115 to 128 in c7a339c
it('read --packageFile', async () => { | |
const stub = stubNpmView('99.9.9', { spawn: true }) | |
const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'npm-check-updates-')) | |
const pkgFile = path.join(tempDir, 'package.json') | |
await fs.writeFile(pkgFile, JSON.stringify({ dependencies: { express: '1' } }), 'utf-8') | |
try { | |
const text = await spawn('node', [bin, '--jsonUpgraded', '--packageFile', pkgFile]) | |
const pkgData = JSON.parse(text) | |
pkgData.should.have.property('express') | |
} finally { | |
await fs.rm(tempDir, { recursive: true, force: true }) | |
stub.restore() | |
} | |
}) |
It makes the tests slightly more verbose, but I've found it easier to manage by having everything related to a test contained in the test file. Plus cleanup is easier because you can just delete the temp directory rather than having to do a precise restore.
Hi, have you had a chance to review the requested changes? Just wanted to check in since some merge conflicts are accumulating as this sits. |
@raineorshine I've fixed the conflict and delivered 2 change requests. But, I have no idea how to deliver the other 2 requests you made. Can I request for your help? |
c9800ed
to
d619d3f
Compare
0bdf754
to
e586f85
Compare
910659d
to
8c370e5
Compare
8c370e5
to
b8c6569
Compare
I added the other targets (minor, patch, semver, greatest, newest), and implemented the list command which is needed for Basic doctor mode works now. Also, I have bun installed conditionally in the CI environment and skip the tests on Windows. I think we can merge this as an initial implementation. Additional functionality can be added in separate PR's. Let me know if there's anything else you'd like to add before I merge. Thanks! |
@raineorshine Nah, All good. Looks Good To Me. Thanks a lot for your help adding bun 🙏 😄 |
@Jarred-Sumner @oven-sh need your help and review