-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: auto-detect package manager in create fuels
#3503
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This PR is published in NPM with version 0.0.0-pr-3503-20241226031927 |
CodSpeed Performance ReportMerging #3503 will degrade performances by 48.61%Comparing Summary
Benchmarks breakdown
|
let packageManager: PackageManager | undefined = cliChosenPackageManagerSelected[0]; | ||
if (cliChosenPackageManagerSelected.length > 1) { | ||
warn('More than one package manager was selected.'); | ||
if (userAgent.startsWith('pnpm')) { |
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.
Nit, please store package manager strings as constants
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.
} | ||
|
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.
console.warn
here if the package users package manager is not supported and we default to npm
?
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.
Coverage Report:
Changed Files:
|
``` | ||
|
||
```sh-vue [bun] | ||
bunx --bun create-fuels@{{fuels}} --bun [project-name] [options] | ||
bunx --bun create-fuels@{{fuels}} [project-name] [options] |
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.
bunx --bun create-fuels@{{fuels}} [project-name] [options] | |
bun create fuels@{{fuels}} [project-name] [options] |
``` | ||
|
||
```sh-vue [bun] | ||
bunx --bun create-fuels@{{fuels}} --bun | ||
bunx --bun create-fuels@{{fuels}} |
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.
bunx --bun create-fuels@{{fuels}} | |
bun create fuels@{{fuels}} |
Running via this command works for me
const packageManager = 'npm'; | ||
const expectedPackageManager = packageMangers[packageManager]; | ||
const { warn } = mockAllDeps(); |
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.
Why remove the mocking here?
Could we not assert that the expected warning is emitted? It also stops the noise in the test outputs (ref)
|
||
const cliChosenPackageManagerSelected = Object.entries(packageMangerOpts) |
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.
Nit: Would looping over the keys to find the package manager be simpler? Then we could remove the name
property from packageMangers
entries.
Object.keys(packageMangers).forEach((packageManager) => {
if (userAgent.startsWith(packageManger)) {
return packageManger as PackageManager;
}
});
warn(`This package manager is not supported. Using npm instead.`);
return packageMangers.npm.name;
create fuels
#3488Release notes
In this release, we:
create fuels
Summary
create fuels
now auto-detects the user's package manger. eg.bun create fuels
will now usebun
to install dependencies.Breaking Changes
These package manager flags have been removed since they are not needed anymore:
--bun
,--pnpm
,--npm
Before
After
Checklist