Skip to content

feat: add support for installing specific versions of packages #4878

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

Open
wants to merge 1 commit into
base: 6.x
Choose a base branch
from
+39 −4
Open

feat: add support for installing specific versions of packages #4878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 13 additions & 4 deletions commands/add.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ export default class Add extends BaseCommand {
@args.string({ description: 'Package name' })
declare name: string

@args.string({ description: 'Package version', required: false })
declare version: string

@flags.boolean({ description: 'Display logs in verbose mode' })
declare verbose?: boolean

Expand Down Expand Up @@ -80,16 +83,20 @@ export default class Add extends BaseCommand {
/**
* Install the package using the selected package manager
*/
async #installPackage(npmPackageName: string) {
async #installPackage(npmPackageName: string, npmPackageVersion: string) {
const colors = this.colors
const spinner = this.logger
.await(`installing ${colors.green(this.name)} using ${colors.grey(this.packageManager!)}`)
.start()

spinner.start()

const packageToInstall = npmPackageVersion
? `${npmPackageName}@${npmPackageVersion}`
: npmPackageName

try {
await installPackage(npmPackageName, {
await installPackage(packageToInstall, {
Comment on lines +94 to +99
Copy link
Author

Choose a reason for hiding this comment

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

This doesn't work for the packages that are filesystem based installs, like those used in the tests. Really the actual installation should probably be mocked out in most of the tests, since we can probably assume that installPackage does its thing.

Copy link
Member

Choose a reason for hiding this comment

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

The reason we don't mock is, because we have been bitten in the past with the install-pkg package. They swapped the underlying fetch library that had different timeout settings and suddenly all installations taking longer than x timeout started failing.

dev: this.dev,
silent: this.verbose === true ? false : true,
cwd: this.app.makePath(),
Expand Down Expand Up @@ -130,7 +137,9 @@ export default class Add extends BaseCommand {
/**
* Prompt the user to confirm the installation
*/
const cmd = colors.grey(`${this.packageManager} add ${this.dev ? '-D ' : ''}${this.name}`)
const cmd = colors.grey(
`${this.packageManager} add ${this.dev ? '-D ' : ''}${this.name}${this.version ? `@${this.version}` : ''}`
)
this.logger.info(`Installing the package using the following command : ${cmd}`)

const shouldInstall = await this.prompt.confirm('Continue ?', {
Expand All @@ -146,7 +155,7 @@ export default class Add extends BaseCommand {
/**
* Install package
*/
const pkgWasInstalled = await this.#installPackage(npmPackageName)
const pkgWasInstalled = await this.#installPackage(npmPackageName, this.version)
if (!pkgWasInstalled) {
return
}
Expand Down
26 changes: 26 additions & 0 deletions tests/commands/add.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,32 @@ test.group('Install', (group) => {
assert.deepEqual(pkgJson.devDependencies, { test: 'file:node_modules/foo' })
})

test('should install specific version of dependency', async ({ assert, fs }) => {
const ace = await new AceFactory().make(fs.baseUrl, {
importer: (filePath) => import(join(filePath, `index.js?${Math.random()}`)),
})

await setupProject(fs, 'npm')
await setupPackage(fs)

const packageVersion = await fs.contentsJson('package.json')
console.log({ packageVersion })
Comment on lines +184 to +185
Copy link
Author

Choose a reason for hiding this comment

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

This was me trying to grab the root package.json so I could just use an already installed package at it's installed version, but uh, that doesn't work due to scoping of the filesystem — I'd probably have to do something different.

Idea was to use like the edge.js package from the root package.json as a node ace add edge.js <version from package.json> type thing, such that we're not hitting the network to install a specific version, whilst avoiding the filesystem path based installs which don't support versions.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can read it manually using the fs package of Node.js vs the test helper?


await ace.app.init()

ace.addLoader(new ListLoader([Configure]))
ace.ui.switchMode('raw')
ace.prompt.trap('install').accept()

const command = await ace.create(Add, [new URL('node_modules/foo', fs.baseUrl).href, 'latest'])
command.verbose = true

await command.exec()

const pkgJson = await fs.contentsJson('package.json')
assert.deepEqual(pkgJson.devDependencies, { test: 'file:node_modules/foo' })
})

test('pass unknown args to configure', async ({ fs, assert }) => {
const ace = await new AceFactory().make(fs.baseUrl, {
importer: (filePath) => import(join(filePath, `index.js?${Math.random()}`)),
Expand Down