Skip to content

Commit

Permalink
Detect pnpm correctly when installing missing dependencies (#37813)
Browse files Browse the repository at this point in the history
This ensures we properly detect `pnpm` when installing missing dependencies. This also adds test coverage to ensure we properly detect the correct package manager. 

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

x-ref: [slack thread](https://vercel.slack.com/archives/C03KAR5DCKC/p1655568091661719)
  • Loading branch information
ijjk authored Jun 19, 2022
1 parent 2313ee0 commit 8577c4f
Show file tree
Hide file tree
Showing 10 changed files with 109 additions and 78 deletions.
11 changes: 7 additions & 4 deletions packages/next/lib/eslint/runLintCheck.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ import { ESLINT_PROMPT_VALUES } from '../constants'
import { existsSync, findPagesDir } from '../find-pages-dir'
import { installDependencies } from '../install-dependencies'
import { hasNecessaryDependencies } from '../has-necessary-dependencies'
import { isYarn } from '../is-yarn'

import * as Log from '../../build/output/log'
import { EventLintCheckCompleted } from '../../telemetry/events/build'
import isError, { getProperError } from '../is-error'
import { getPkgManager } from '../helpers/get-pkg-manager'

type Config = {
plugins: string[]
Expand Down Expand Up @@ -99,15 +99,18 @@ async function lint(
try {
// Load ESLint after we're sure it exists:
const deps = await hasNecessaryDependencies(baseDir, requiredPackages)
const packageManager = getPkgManager(baseDir)

if (deps.missing.some((dep) => dep.pkg === 'eslint')) {
Log.error(
`ESLint must be installed${
lintDuringBuild ? ' in order to run during builds:' : ':'
} ${chalk.bold.cyan(
(await isYarn(baseDir))
? 'yarn add --dev eslint'
: 'npm install --save-dev eslint'
(packageManager === 'yarn'
? 'yarn add --dev'
: packageManager === 'pnpm'
? 'pnpm install --save-dev'
: 'npm install --save-dev') + ' eslint'
)}`
)
return null
Expand Down
36 changes: 36 additions & 0 deletions packages/next/lib/helpers/get-pkg-manager.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import fs from 'fs'
import path from 'path'
import { execSync } from 'child_process'

export type PackageManager = 'npm' | 'pnpm' | 'yarn'

export function getPkgManager(baseDir: string): PackageManager {
try {
for (const { lockFile, packageManager } of [
{ lockFile: 'yarn.lock', packageManager: 'yarn' },
{ lockFile: 'pnpm-lock.yaml', packageManager: 'pnpm' },
{ lockFile: 'package-lock.json', packageManager: 'npm' },
]) {
if (fs.existsSync(path.join(baseDir, lockFile))) {
return packageManager as PackageManager
}
}
const userAgent = process.env.npm_config_user_agent
if (userAgent) {
if (userAgent.startsWith('yarn')) {
return 'yarn'
} else if (userAgent.startsWith('pnpm')) {
return 'pnpm'
}
}
try {
execSync('yarn --version', { stdio: 'ignore' })
return 'yarn'
} catch {
execSync('pnpm --version', { stdio: 'ignore' })
return 'pnpm'
}
} catch {
return 'npm'
}
}
27 changes: 12 additions & 15 deletions packages/next/lib/helpers/install.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
/* eslint-disable import/no-extraneous-dependencies */
import chalk from 'next/dist/compiled/chalk'
import spawn from 'next/dist/compiled/cross-spawn'

export type PackageManager = 'npm' | 'pnpm' | 'yarn'

interface InstallArgs {
/**
* Indicate whether to install packages using Yarn.
* Indicate whether to install packages using npm, pnpm or Yarn.
*/
useYarn: boolean
packageManager: PackageManager
/**
* Indicate whether there is an active Internet connection.
*/
Expand All @@ -25,10 +26,10 @@ interface InstallArgs {
export function install(
root: string,
dependencies: string[] | null,
{ useYarn, isOnline, devDependencies }: InstallArgs
{ packageManager, isOnline, devDependencies }: InstallArgs
): Promise<void> {
/**
* NPM-specific command-line flags.
* (p)npm-specific command-line flags.
*/
const npmFlags: string[] = []
/**
Expand All @@ -40,11 +41,12 @@ export function install(
*/
return new Promise((resolve, reject) => {
let args: string[]
let command: string = useYarn ? 'yarnpkg' : 'npm'
let command = packageManager
const useYarn = packageManager === 'yarn'

if (dependencies && dependencies.length) {
/**
* If there are dependencies, run a variation of `{displayCommand} add`.
* If there are dependencies, run a variation of `{packageManager} add`.
*/
if (useYarn) {
/**
Expand All @@ -57,15 +59,15 @@ export function install(
args.push(...dependencies)
} else {
/**
* Call `npm install [--save|--save-dev] ...`.
* Call `(p)npm install [--save|--save-dev] ...`.
*/
args = ['install', '--save-exact']
args.push(devDependencies ? '--save-dev' : '--save')
args.push(...dependencies)
}
} else {
/**
* If there are no dependencies, run a variation of `{displayCommand}
* If there are no dependencies, run a variation of `{packageManager}
* install`.
*/
args = ['install']
Expand Down Expand Up @@ -93,12 +95,7 @@ export function install(
*/
const child = spawn(command, args, {
stdio: 'inherit',
env: {
...process.env,
NODE_ENV: devDependencies ? 'development' : 'production',
ADBLOCK: '1',
DISABLE_OPENCOLLECTIVE: '1',
},
env: { ...process.env, ADBLOCK: '1', DISABLE_OPENCOLLECTIVE: '1' },
})
child.on('close', (code) => {
if (code !== 0) {
Expand Down
22 changes: 0 additions & 22 deletions packages/next/lib/helpers/should-use-yarn.ts

This file was deleted.

15 changes: 9 additions & 6 deletions packages/next/lib/install-dependencies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import chalk from 'next/dist/compiled/chalk'
import path from 'path'

import { MissingDependency } from './has-necessary-dependencies'
import { shouldUseYarn } from './helpers/should-use-yarn'
import { getPkgManager } from './helpers/get-pkg-manager'
import { install } from './helpers/install'
import { getOnline } from './helpers/get-online'

Expand All @@ -15,22 +15,25 @@ export async function installDependencies(
deps: any,
dev: boolean = false
) {
const useYarn = shouldUseYarn(baseDir)
const isOnline = !useYarn || (await getOnline())
const packageManager = getPkgManager(baseDir)
const isOnline = await getOnline()

if (deps.length) {
console.log()
console.log(`Installing ${dev ? 'devDependencies' : 'dependencies'}:`)
console.log(
`Installing ${
dev ? 'devDependencies' : 'dependencies'
} (${packageManager}):`
)
for (const dep of deps) {
console.log(`- ${chalk.cyan(dep.pkg)}`)
}
console.log()

const devInstallFlags = { devDependencies: dev, ...{ useYarn, isOnline } }
await install(
path.resolve(baseDir),
deps.map((dep: MissingDependency) => dep.pkg),
devInstallFlags
{ devDependencies: dev, isOnline, packageManager }
)
console.log()
}
Expand Down
5 changes: 0 additions & 5 deletions packages/next/lib/is-yarn.ts

This file was deleted.

9 changes: 7 additions & 2 deletions packages/next/lib/typescript/missingDependencyError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ import chalk from 'next/dist/compiled/chalk'
import { getOxfordCommaList } from '../oxford-comma-list'
import { MissingDependency } from '../has-necessary-dependencies'
import { FatalError } from '../fatal-error'
import { isYarn } from '../is-yarn'
import { getPkgManager } from '../helpers/get-pkg-manager'

export async function missingDepsError(
dir: string,
missingPackages: MissingDependency[]
) {
const packagesHuman = getOxfordCommaList(missingPackages.map((p) => p.pkg))
const packagesCli = missingPackages.map((p) => p.pkg).join(' ')
const packageManager = getPkgManager(dir)

const removalMsg =
'\n\n' +
Expand All @@ -28,7 +29,11 @@ export async function missingDepsError(
chalk.bold(`Please install ${chalk.bold(packagesHuman)} by running:`) +
'\n\n' +
`\t${chalk.bold.cyan(
((await isYarn(dir)) ? 'yarn add --dev' : 'npm install --save-dev') +
(packageManager === 'yarn'
? 'yarn add --dev'
: packageManager === 'pnpm'
? 'pnpm install --save-dev'
: 'npm install --save-dev') +
' ' +
packagesCli
)}` +
Expand Down
12 changes: 8 additions & 4 deletions packages/next/lib/verify-partytown-setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ import {
hasNecessaryDependencies,
NecessaryDependencies,
} from './has-necessary-dependencies'
import { isYarn } from './is-yarn'
import { fileExists } from './file-exists'
import { FatalError } from './fatal-error'
import { recursiveDelete } from './recursive-delete'
import * as Log from '../build/output/log'
import { getPkgManager } from './helpers/get-pkg-manager'

async function missingDependencyError(dir: string) {
const packageManager = getPkgManager(dir)

throw new FatalError(
chalk.bold.red(
"It looks like you're trying to use Partytown with next/script but do not have the required package(s) installed."
Expand All @@ -21,9 +23,11 @@ async function missingDependencyError(dir: string) {
chalk.bold(`Please install Partytown by running:`) +
'\n\n' +
`\t${chalk.bold.cyan(
(await isYarn(dir))
? 'yarn add @builder.io/partytown'
: 'npm install @builder.io/partytown'
(packageManager === 'yarn'
? 'yarn add --dev'
: packageManager === 'pnpm'
? 'pnpm install --save-dev'
: 'npm install --save-dev') + ' @builder.io/partytown'
)}` +
'\n\n' +
chalk.bold(
Expand Down
46 changes: 28 additions & 18 deletions test/integration/eslint/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,24 +185,23 @@ describe('ESLint', () => {

describe('Next Lint', () => {
describe('First Time Setup ', () => {
async function nextLintTemp() {
async function nextLintTemp(setupCallback) {
const folder = join(
os.tmpdir(),
Math.random().toString(36).substring(2)
)
await fs.mkdirp(folder)
await fs.copy(dirNoConfig, folder)
await setupCallback?.(folder)

try {
const nextDir = dirname(require.resolve('next/package'))
const nextBin = join(nextDir, 'dist/bin/next')
const { stdout, stderr } = await nextLint(folder, ['--strict'], {
stderr: true,
stdout: true,
cwd: folder,
})

const { stdout } = await execa('node', [
nextBin,
'lint',
folder,
'--strict',
])
console.log({ stdout, stderr })

const pkgJson = JSON.parse(
await fs.readFile(join(folder, 'package.json'), 'utf8')
Expand Down Expand Up @@ -234,15 +233,26 @@ describe('ESLint', () => {
expect(output).toContain('Cancel')
})

test('installs eslint and eslint-config-next as devDependencies if missing', async () => {
const { stdout, pkgJson } = await nextLintTemp()

expect(stdout.replace(/(\r\n|\n|\r)/gm, '')).toContain(
'Installing devDependencies:- eslint- eslint-config-next'
)
expect(pkgJson.devDependencies).toHaveProperty('eslint')
expect(pkgJson.devDependencies).toHaveProperty('eslint-config-next')
})
for (const { packageManger, lockFile } of [
{ packageManger: 'yarn', lockFile: 'yarn.lock' },
{ packageManger: 'pnpm', lockFile: 'pnpm-lock.yaml' },
{ packageManger: 'npm', lockFile: 'package-lock.json' },
]) {
test(`installs eslint and eslint-config-next as devDependencies if missing with ${packageManger}`, async () => {
const { stdout, pkgJson } = await nextLintTemp(async (folder) => {
await fs.writeFile(join(folder, lockFile), '')
})

expect(stdout).toContain(
`Installing devDependencies (${packageManger}):`
)
expect(stdout).toContain('eslint')
expect(stdout).toContain('eslint-config-next')
expect(stdout).toContain(packageManger)
expect(pkgJson.devDependencies).toHaveProperty('eslint')
expect(pkgJson.devDependencies).toHaveProperty('eslint-config-next')
})
}

test('creates .eslintrc.json file with a default configuration', async () => {
const { stdout, eslintrcJson } = await nextLintTemp()
Expand Down
4 changes: 2 additions & 2 deletions test/integration/script-loader/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,8 @@ describe('Next.js Script - Primary Strategies', () => {
})
const output = stdout + stderr

expect(output.replace(/\n|\r/g, '')).toContain(
`It looks like you're trying to use Partytown with next/script but do not have the required package(s) installed.Please install Partytown by running: npm install @builder.io/partytownIf you are not trying to use Partytown, please disable the experimental "nextScriptWorkers" flag in next.config.js.`
expect(output.replace(/\n|\r/g, '')).toMatch(
/It looks like you're trying to use Partytown with next\/script but do not have the required package\(s\) installed.Please install Partytown by running:.*?(npm|pnpm|yarn) (install|add) (--save-dev|--dev) @builder.io\/partytownIf you are not trying to use Partytown, please disable the experimental "nextScriptWorkers" flag in next.config.js./
)
})
})

0 comments on commit 8577c4f

Please sign in to comment.