Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {loadLocalExtensionsSpecifications} from '../extensions/load-specificatio
import {handleWatcherEvents} from '../../services/dev/app-events/app-event-watcher-handler.js'
import {EventType} from '../../services/dev/app-events/app-event-watcher.js'
import {describe, expect, test} from 'vitest'
import {inTemporaryDirectory, writeFile, mkdir} from '@shopify/cli-kit/node/fs'
import {inTemporaryDirectory, writeFile, mkdir, removeFile} from '@shopify/cli-kit/node/fs'
import {joinPath} from '@shopify/cli-kit/node/path'
import {AbortController} from '@shopify/cli-kit/node/abort'

Expand Down Expand Up @@ -199,6 +199,19 @@ describe('Project integration', () => {
})
})

test('Project uses unknown package manager metadata when the app root has no package.json', async () => {
await inTemporaryDirectory(async (dir) => {
await setupRealApp(dir)
await removeFile(joinPath(dir, 'package.json'))

const project = await Project.load(dir)

expect(project.packageManager).toBe('unknown')
expect(project.nodeDependencies).toStrictEqual({})
expect(project.usesWorkspaces).toBe(false)
})
})

test('multi-config project discovers all configs', async () => {
await inTemporaryDirectory(async (dir) => {
await setupRealApp(dir)
Expand Down
10 changes: 5 additions & 5 deletions packages/app/src/cli/models/project/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import {readAndParseDotEnv, DotEnvFile} from '@shopify/cli-kit/node/dot-env'
import {fileExists, glob, findPathUp, readFile} from '@shopify/cli-kit/node/fs'
import {
getDependencies,
getPackageManager,
PackageManager,
getPackageManagerForProjectRoot,
ProjectPackageManager,
usesWorkspaces as detectUsesWorkspaces,
} from '@shopify/cli-kit/node/node-package-manager'
import {joinPath, basename} from '@shopify/cli-kit/node/path'
Expand Down Expand Up @@ -76,7 +76,7 @@ export class Project {
// Project metadata
const packageJSONPath = joinPath(directory, 'package.json')
const hasPackageJson = await fileExists(packageJSONPath)
const packageManager = hasPackageJson ? await getPackageManager(directory) : 'unknown'
const packageManager = hasPackageJson ? await getPackageManagerForProjectRoot(directory) : 'unknown'
const nodeDependencies = hasPackageJson ? await getDependencies(packageJSONPath) : {}
const usesWorkspaces = hasPackageJson ? await detectUsesWorkspaces(directory) : false

Expand All @@ -101,7 +101,7 @@ export class Project {
}

readonly directory: string
readonly packageManager: PackageManager
readonly packageManager: ProjectPackageManager | 'unknown'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you remove unknown and either make the operation sync or force the caller to narrow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I looked at that. I kept 'unknown' here because Project is still just modeling what’s on disk, and “app root with no package.json” is a real state.

The sync narrowing is in the next PR which adds requirePackageManagerForOperations(), so install/mutation callers have to narrow before using it.

Happy to fold that in here instead if you’d prefer, but I split it to keep metadata vs operation-owned use separate.

readonly nodeDependencies: Record<string, string>
readonly usesWorkspaces: boolean
readonly appConfigFiles: TomlFile[]
Expand All @@ -119,7 +119,7 @@ export class Project {

private constructor(options: {
directory: string
packageManager: PackageManager
packageManager: ProjectPackageManager | 'unknown'
nodeDependencies: Record<string, string>
usesWorkspaces: boolean
appConfigFiles: TomlFile[]
Expand Down
55 changes: 55 additions & 0 deletions packages/cli-kit/src/public/node/node-package-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
addResolutionOrOverride,
writePackageJSON,
getPackageManager,
getPackageManagerForProjectRoot,
packageManagerBinaryCommandForDirectory,
installNPMDependenciesRecursively,
addNPMDependencies,
Expand Down Expand Up @@ -784,6 +785,23 @@ describe('addResolutionOrOverride', () => {
})
})

test('when package.json has no lockfile then npm overrides are used by default', async () => {
await inTemporaryDirectory(async (tmpDir) => {
// Given
const reactType = {'@types/react': '17.0.30'}
const packageJsonPath = joinPath(tmpDir, 'package.json')
await writeFile(packageJsonPath, JSON.stringify({}))

// When
await addResolutionOrOverride(tmpDir, reactType)

// Then
const packageJsonContent = await readAndParsePackageJson(packageJsonPath)
expect(packageJsonContent.overrides).toEqual(reactType)
expect(packageJsonContent.resolutions).toBeUndefined()
})
})

test('when package.json with existing resolution type and yarn manager then dependency version is overwritten', async () => {
await inTemporaryDirectory(async (tmpDir) => {
// Given
Expand Down Expand Up @@ -951,6 +969,43 @@ describe('getPackageManager', () => {
})
})

describe('getPackageManagerForProjectRoot', () => {
test('detects the package manager from root markers without consulting the user agent', async () => {
await inTemporaryDirectory(async (tmpDir) => {
await writePackageJSON(tmpDir, {name: 'mock name'})
await writeFile(joinPath(tmpDir, 'yarn.lock'), '')
vi.stubEnv('npm_config_user_agent', 'pnpm/9.0.0')

try {
await expect(getPackageManagerForProjectRoot(tmpDir)).resolves.toBe('yarn')
} finally {
vi.unstubAllEnvs()
}
})
})

test('defaults to npm when the project root has a package.json but no lockfile markers', async () => {
await inTemporaryDirectory(async (tmpDir) => {
await writePackageJSON(tmpDir, {name: 'mock name'})
vi.stubEnv('npm_config_user_agent', 'pnpm/9.0.0')

try {
await expect(getPackageManagerForProjectRoot(tmpDir)).resolves.toBe('npm')
} finally {
vi.unstubAllEnvs()
}
})
})

test('throws when the provided root does not contain a package.json', async () => {
await inTemporaryDirectory(async (tmpDir) => {
await expect(getPackageManagerForProjectRoot(tmpDir)).rejects.toThrow(
new PackageJsonNotFoundError(normalizePath(tmpDir)),
)
})
})
})

describe('packageManagerBinaryCommandForDirectory', () => {
test('uses npm exec with -- for npm', async () => {
await inTemporaryDirectory(async (tmpDir) => {
Expand Down
43 changes: 35 additions & 8 deletions packages/cli-kit/src/public/node/node-package-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export type DependencyType = 'dev' | 'prod' | 'peer'
*/
export const packageManager = ['yarn', 'npm', 'pnpm', 'bun', 'homebrew', 'unknown'] as const
export type PackageManager = (typeof packageManager)[number]
type ProjectPackageManager = Extract<PackageManager, 'yarn' | 'npm' | 'pnpm' | 'bun'>
export type ProjectPackageManager = Extract<PackageManager, 'yarn' | 'npm' | 'pnpm' | 'bun'>

/**
* Returns an abort error that's thrown when the package manager can't be determined.
Expand Down Expand Up @@ -145,6 +145,17 @@ function packageManagerBinaryCommand(
}
}

function getProjectPackageManagerAtDirectory(directory: string): ProjectPackageManager | undefined {
if (fileExistsSync(joinPath(directory, yarnLockfile))) return 'yarn'
if (fileExistsSync(joinPath(directory, pnpmLockfile)) || fileExistsSync(joinPath(directory, pnpmWorkspaceFile))) {
return 'pnpm'
}
if (hasBunLockfileSync(directory)) return 'bun'
if (fileExistsSync(joinPath(directory, npmLockfile))) return 'npm'

return undefined
}

/**
* Returns the dependency manager used in a directory.
* Walks upward from `fromDirectory` so workspace packages (e.g. `extensions/my-fn/package.json`)
Expand All @@ -158,12 +169,9 @@ export async function getPackageManager(fromDirectory: string): Promise<PackageM
let current = fromDirectory
outputDebug(outputContent`Looking for a lockfile in ${outputToken.path(current)}...`)
while (true) {
if (fileExistsSync(joinPath(current, yarnLockfile))) return 'yarn'
if (fileExistsSync(joinPath(current, pnpmLockfile)) || fileExistsSync(joinPath(current, pnpmWorkspaceFile))) {
return 'pnpm'
}
if (hasBunLockfileSync(current)) return 'bun'
if (fileExistsSync(joinPath(current, npmLockfile))) return 'npm'
const detectedPackageManager = getProjectPackageManagerAtDirectory(current)
if (detectedPackageManager) return detectedPackageManager

const parent = dirname(current)
if (parent === current) break
current = parent
Expand All @@ -175,6 +183,25 @@ export async function getPackageManager(fromDirectory: string): Promise<PackageM
return 'npm'
}

/**
* Resolves the package manager for a directory already known to be the project root.
*
* Use this when the caller already knows the root directory and wants to inspect that root
* directly rather than walking upward from a child directory.
*
* @param rootDirectory - The known project root directory.
* @returns The package manager detected from root markers, defaulting to `npm` when no marker exists.
* @throws PackageJsonNotFoundError if the provided directory does not contain a package.json.
*/
export async function getPackageManagerForProjectRoot(rootDirectory: string): Promise<ProjectPackageManager> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When you work with the CLI, I think you typically run it from the project root or from an extension. But both of them already have a package.json, so why using this instead of getPackageManager? Isn't it the same?

const packageJsonPath = joinPath(rootDirectory, 'package.json')
if (!(await fileExists(packageJsonPath))) {
throw new PackageJsonNotFoundError(rootDirectory)
}

return getProjectPackageManagerAtDirectory(rootDirectory) ?? 'npm'
}

/**
* Builds the command and argv needed to execute a local binary using the package manager
* detected from the provided directory or its ancestors.
Expand Down Expand Up @@ -733,7 +760,7 @@ export async function findUpAndReadPackageJson(fromDirectory: string): Promise<{
}

export async function addResolutionOrOverride(directory: string, dependencies: Record<string, string>): Promise<void> {
const packageManager = await getPackageManager(directory)
const packageManager = await getPackageManagerForProjectRoot(directory)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How do you know that this directory is the root or not? If it must be, I'd make it explicit with the param name or documentation.

const packageJsonPath = joinPath(directory, 'package.json')
const packageJsonContent = await readAndParsePackageJson(packageJsonPath)

Expand Down
4 changes: 2 additions & 2 deletions packages/cli-kit/src/public/node/upgrade.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
DependencyType,
usesWorkspaces,
addNPMDependencies,
getPackageManager,
getPackageManagerForProjectRoot,
} from './node-package-manager.js'
import {outputContent, outputDebug, outputInfo, outputToken, outputWarn} from './output.js'
import {cwd, moduleDirectory, sniffForPath} from './path.js'
Expand Down Expand Up @@ -210,7 +210,7 @@ async function installJsonDependencies(

if (packagesToUpdate.length > 0) {
await addNPMDependencies(packagesToUpdate, {
packageManager: await getPackageManager(directory),
packageManager: await getPackageManagerForProjectRoot(directory),
type: depsEnv,
directory,
stdout: process.stdout,
Expand Down
Loading