Skip to content

Commit

Permalink
fix: issue npm#7892 - fix for npm install creating directories and em…
Browse files Browse the repository at this point in the history
…pty package.json file
  • Loading branch information
Kyle-Ignis committed Nov 25, 2024
1 parent 6995303 commit 4fccaa7
Show file tree
Hide file tree
Showing 6 changed files with 242 additions and 11 deletions.
16 changes: 16 additions & 0 deletions lib/commands/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const pacote = require('pacote')
const checks = require('npm-install-checks')
const reifyFinish = require('../utils/reify-finish.js')
const ArboristWorkspaceCmd = require('../arborist-cmd.js')
const validateProjectStructure = require('../utils/validate-project.js')

class Install extends ArboristWorkspaceCmd {
static description = 'Install a package'
Expand Down Expand Up @@ -104,6 +105,21 @@ class Install extends ArboristWorkspaceCmd {
const forced = this.npm.config.get('force')
const scriptShell = this.npm.config.get('script-shell') || undefined

// Add validation for non-global installs with no args
if (!isGlobalInstall && args.length === 0) {
try {
validateProjectStructure(this.npm.prefix)
} catch (err) {
if (err.code === 'ENOPROJECT') {
throw Object.assign(
new Error(err.message),
{ code: err.code }
)
}
throw err
}
}

// be very strict about engines when trying to update npm itself
const npmInstall = args.find(arg => arg.startsWith('npm@') || arg === 'npm')
if (isGlobalInstall && npmInstall) {
Expand Down
16 changes: 16 additions & 0 deletions lib/commands/uninstall.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const pkgJson = require('@npmcli/package-json')
const reifyFinish = require('../utils/reify-finish.js')
const completion = require('../utils/installed-shallow.js')
const ArboristWorkspaceCmd = require('../arborist-cmd.js')
const validateProjectStructure = require('../utils/validate-project.js')

class Uninstall extends ArboristWorkspaceCmd {
static description = 'Remove a package'
Expand All @@ -18,6 +19,21 @@ class Uninstall extends ArboristWorkspaceCmd {
}

async exec (args) {
// Add validation for non-global uninstalls
if (!this.npm.global) {
try {
validateProjectStructure(this.npm.prefix)
} catch (err) {
if (err.code === 'ENOPROJECT') {
throw Object.assign(
new Error(err.message),
{ code: err.code }
)
}
throw err
}
}

if (!args.length) {
if (!this.npm.global) {
throw new Error('Must provide a package name to remove')
Expand Down
26 changes: 26 additions & 0 deletions lib/utils/validate-project.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
const fs = require('node:fs')
const path = require('node:path')

// Validates that a package.json exists in the target directory
function validateProjectStructure (prefix) {
const projectPath = prefix || process.cwd()
const packageJsonPath = path.join(projectPath, 'package.json')

// Check if directory exists when --prefix is used
if (prefix && !fs.existsSync(projectPath)) {
const err = new Error(`Dir "${projectPath}" does not exist. Run "npm init" first.`)
err.code = 'ENOPROJECT'
throw err
}

// Check for package.json
if (!fs.existsSync(packageJsonPath)) {
const err = new Error('No package.json found. Run "npm init" to create a new package.')
err.code = 'ENOPROJECT'
throw err
}

return true
}

module.exports = validateProjectStructure
44 changes: 44 additions & 0 deletions test/lib/commands/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -717,3 +717,47 @@ t.test('devEngines', async t => {
t.ok(!output.includes('EBADDEVENGINES'))
})
})

t.test('package.json validation error handling', async t => {
await t.test('handles ENOPROJECT errors', async t => {
const { npm } = await loadMockNpm(t, {
prefixDir: {},
mocks: {
'{LIB}/utils/validate-project.js': () => {
const err = new Error('Custom error')
err.code = 'ENOPROJECT'
throw err
},
},
})
await t.rejects(
npm.exec('install', []),
{
code: 'ENOPROJECT',
message: 'Custom error',
},
'should preserve error code and message'
)
})

await t.test('handles non-ENOPROJECT errors', async t => {
const { npm } = await loadMockNpm(t, {
prefixDir: {},
mocks: {
'{LIB}/utils/validate-project.js': () => {
const err = new Error('Different error')
err.code = 'EDIFFERENT'
throw err
},
},
})
await t.rejects(
npm.exec('install', []),
{
code: 'EDIFFERENT',
message: 'Different error',
},
'should throw through other errors unchanged'
)
})
})
104 changes: 93 additions & 11 deletions test/lib/commands/uninstall.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,14 @@ t.test('remove single installed lib', async t => {
t.test('remove multiple installed libs', async t => {
const { uninstall, prefix } = await mockNpm(t, {
prefixDir: {
'package.json': JSON.stringify({
name: 'test-rm-multiple-lib',
version: '1.0.0',
dependencies: {
a: '*',
b: '*',
},
}),
node_modules: {
a: {
'package.json': JSON.stringify({
Expand Down Expand Up @@ -139,18 +147,8 @@ t.test('remove multiple installed libs', async t => {

await uninstall(['b'])

t.throws(() => fs.statSync(a), 'should have removed a package from nm')
t.throws(() => fs.statSync(b), 'should have removed b package from nm')
})

t.test('no args local', async t => {
const { uninstall } = await mockNpm(t)

await t.rejects(
uninstall([]),
/Must provide a package name to remove/,
'should throw package name required error'
)
t.ok(fs.statSync(a), 'should not have removed a package from nm')
})

t.test('no args global', async t => {
Expand Down Expand Up @@ -200,3 +198,87 @@ t.test('non ENOENT error reading from localPrefix package.json', async t => {
'should throw non ENOENT error'
)
})

t.test('package.json validation', async t => {
await t.test('no package.json in local uninstall', async t => {
const { uninstall } = await mockNpm(t, {
prefixDir: {}, // empty directory
})
await t.rejects(
uninstall(['some-package']),
{
code: 'ENOPROJECT',
message: 'No package.json found. Run "npm init" to create a new package.',
},
'should throw ENOPROJECT error'
)
})

await t.test('no package.json in local uninstall', async t => {
const { uninstall } = await mockNpm(t, {
prefixDir: {}, // empty directory
})
await t.rejects(
uninstall(['some-package']),
{
code: 'ENOPROJECT',
message: 'No package.json found. Run "npm init" to create a new package.',
}
)
})
})

t.test('validation error handling', async t => {
const { uninstall } = await mockNpm(t, {
prefixDir: {},
mocks: {
'{LIB}/utils/validate-project.js': () => {
throw new Error('Generic error')
},
},
})
await t.rejects(
uninstall(['some-package']),
/Generic error/,
'should throw through non-ENOPROJECT errors'
)
})

t.test('no args with package name validation', async t => {
const { uninstall } = await mockNpm(t, {
prefixDir: {
'package.json': JSON.stringify({
name: 'test-pkg',
version: '1.0.0',
}),
},
})
await t.rejects(
uninstall([]),
{
message: 'Must provide a package name to remove',
},
'should throw correct error message'
)
})

t.test('handles non-ENOPROJECT validation errors', async t => {
const { uninstall } = await mockNpm(t, {
prefixDir: {},
mocks: {
'{LIB}/utils/validate-project.js': () => {
const err = new Error('Different error')
err.code = 'EDIFFERENT'
throw err
},
},
})
await t.rejects(
uninstall(['some-package']),
{
code: 'EDIFFERENT',
message: 'Different error',
},
'should throw through other errors unchanged'
)
})
47 changes: 47 additions & 0 deletions test/lib/commands/validate-project.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
const t = require('tap')

// Mock fs.existsSync to control file existence checks
const mockFs = {
existsSync: () => true,
}

const validate = t.mock('../../../lib/utils/validate-project.js', {
'node:fs': mockFs,
})

t.test('validate project structure', async t => {
t.test('returns true when package.json exists', async t => {
mockFs.existsSync = () => true
t.equal(validate('/some/path'), true, 'should validate successfully')
})

t.test('uses cwd() when no prefix provided', async t => {
mockFs.existsSync = () => true
t.equal(validate(), true, 'should validate successfully with default path')
})

t.test('throws ENOPROJECT when directory does not exist', async t => {
mockFs.existsSync = () => false
t.throws(
() => validate('/non-existent-dir'),
{
code: 'ENOPROJECT',
message: 'Dir "/non-existent-dir" does not exist. Run "npm init" to begin.',
},
'should throw correct error for missing directory'
)
})

t.test('throws ENOPROJECT when package.json is missing', async t => {
// Directory exists but package.json doesn't
mockFs.existsSync = (p) => !p.endsWith('package.json')
t.throws(
() => validate('/some/path'),
{
code: 'ENOPROJECT',
message: 'No package.json found. Run "npm init" to create a new package.',
},
'should throw correct error for missing package.json'
)
})
})

0 comments on commit 4fccaa7

Please # to comment.