Add yarn berry (v2+) support to instalNodeModules #6199
Conversation
Coverage report
Show files with reduced coverage 🔻
Test suite run success3135 tests passing in 1328 suites. Report generated by 🧪jest coverage report action from 11b6da3 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Yarn Berry (v2+) to the installNodeModules function by implementing version-aware command selection. Previously, the function hardcoded the 'install' command for all package managers, causing build failures with modern Yarn versions that use the 'add' command.
- Implements a new
determineYarnInstallCommandfunction with multi-tier version detection strategy - Updates
installNodeModulesto use yarn-specific logic while maintaining backward compatibility - Adds comprehensive test coverage for all detection scenarios and fallback mechanisms
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/cli-kit/src/public/node/node-package-manager.ts | Adds yarn version detection logic and updates installNodeModules function |
| packages/cli-kit/src/public/node/node-package-manager.test.ts | Comprehensive test suite for yarn version detection and install command selection |
| .changeset/gold-needles-kiss.md | Changeset documenting the yarn berry compatibility addition |
Comments suppressed due to low confidence (1)
packages/cli-kit/src/public/node/node-package-manager.ts:193
- The function name 'determineYarnInstallCommand' is misleading - it returns both 'install' and 'add' commands, not just install commands. Consider renaming to 'getYarnCommand' or 'determineYarnCommand'.
export async function determineYarnInstallCommand(directory: string): Promise<string[]> {
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/node-package-manager.d.ts@@ -87,6 +87,7 @@ interface InstallNPMDependenciesRecursivelyOptions {
* @param options - Options to install dependencies recursively.
*/
export declare function installNPMDependenciesRecursively(options: InstallNPMDependenciesRecursivelyOptions): Promise<void>;
+export declare function determineYarnInstallCommand(directory: string): Promise<string[]>;
interface InstallNodeModulesOptions {
directory: string;
args?: string[];
@@ -219,6 +220,11 @@ export interface PackageJson {
* https://docs.npmjs.com/cli/v9/configuring-npm/package-json#private
*/
private?: boolean;
+ /**
+ * The packageManager attribute of the package.json.
+ * https://docs.npmjs.com/cli/v9/configuring-npm/package-json#packagemanager
+ */
+ packageManager?: string;
}
/**
* Reads and parses a package.json
|
|
|
||
| let args: string[] | ||
| if (options.packageManager === 'yarn') { | ||
| args = await determineYarnInstallCommand(options.directory) |
There was a problem hiding this comment.
I don't think this approach works, because it looks like it would break cases where installNodeModules is called with just flags but no dependencies to add (eg: yarn install --force). From my understanding:
- both yarn v1 and yarn berry use
yarn installto just install dependencies (ie: not add, just install) - To ADD dependencies:
- yarn v1 supports
yarn install <package>ORyarn add <package> - yarn berry ONLY supports
yarn add <package>
- yarn v1 supports
So this seems like a much cleaner and simpler solution:
let args: string[]
// Are all of the args just flags (like `--force`)? Or are any of them actual packages to ADD?
const hasPackageArgs = args?.some(arg => !arg.startsWith('-'))
if (hasPackageArgs && options.packageManager === 'yarn') {
args = ['add']
} else {
args = ['install']
}
if (options.args) {
args = args.concat(options.args)
}
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |
WHY are these changes introduced?
The
installNodeModulesfunction in cli-kit hardcoded the ['install'] command for all package managers, including yarn. This caused build failures when using Yarn Berry (v2/v3+) because:This incompatibility blocked the Hydrogen CLI upgrade command and other cli-kit consumers from working with modern yarn versions, forcing users to stay on legacy yarn v1 or face broken installations.
WHAT is this pull request doing?
Enhanced installNodeModules with yarn version-aware command selection:
New Function: getYarnInstallCommand(directory: string)
Enhanced installNodeModules Function
Detection Strategy (Priority Order)
How to test your changes?
Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist