Skip to content
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.

Sign up for GitHub

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

fix: don't emit package-lock.json when package.json does not exist #7097

Open
wants to merge 4 commits into
base: latest
Choose a base branch
from
Open

fix: don't emit package-lock.json when package.json does not exist #7097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
909cfab
fix: don't reify if package.json does not exist or is empty object
xiongmao86 Jan 7, 2024
1d965ae
fix: add two tests
xiongmao86 Jan 8, 2024
0acd2a2
fix: add content to package.json to pass test
xiongmao86 Jan 8, 2024
ef640d8
fix: add name to package.json to avoid throw
xiongmao86 Jan 9, 2024
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
16 changes: 16 additions & 0 deletions lib/commands/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const { resolve, join } = require('path')
const runScript = require('@npmcli/run-script')
const pacote = require('pacote')
const checks = require('npm-install-checks')
const PackageJson = require('@npmcli/package-json')

const ArboristWorkspaceCmd = require('../arborist-cmd.js')
class Install extends ArboristWorkspaceCmd {
Expand Down Expand Up @@ -140,6 +141,21 @@ class Install extends ArboristWorkspaceCmd {
throw this.usageError()
}

if (!isGlobalInstall) {
const pkgJson = new PackageJson()
await pkgJson.load(this.npm.prefix)
const jsonContent = pkgJson.content
const isJsonEmptyObject = Object.keys(jsonContent).length === 0
&& jsonContent.constructor === Object

const emptyJson = new Error('EEMPTYPKGJSON: package.json is empty object')
Copy link
Contributor

Choose a reason for hiding this comment

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

why not remove this check, let it generate empty lock file if package.json is empty. Only make sure not to generate lock file if package.json file doesn't exists

Copy link
Member

@wraithgar wraithgar Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If you'll read my (admittedly long) comment you'll see that this check in this place is not the right solution to begin with.

emptyJson.code = 'EEMPTYPKGJSON'
emptyJson.path = pkgJson.filename
if (isJsonEmptyObject) {
throw emptyJson
}
Comment on lines +147 to +156
Copy link
Author

Choose a reason for hiding this comment

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

Hi, @wraithgar, that code do you mean this snippet? But it's written by me, not left over before I wrote the commit? I still don't understand, Can you point to one place of the code for example?

Copy link
Member

@wraithgar wraithgar Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was trying to infer why the test for an empty object was there and assumed it was due to rpj. The long and the short of it is we don't need that check, either package-json returns or it doesn't, if it returns at all we read a package.json file from somewhere in the tree.

}

const Arborist = require('@npmcli/arborist')
const opts = {
...this.npm.flatOptions,
Expand Down
8 changes: 8 additions & 0 deletions lib/utils/error-message.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,14 @@ const errorMessage = (er, npm) => {
])
break

case 'EEMPTYPKGJSON':
short.push(['eemptypkgjson', er.message])
detail.push([
'eemptypkgjson',
'The root of package.json is an empty object',
])
break

default:
short.push(['', er.message || er])
if (er.signal) {
Expand Down
9 changes: 9 additions & 0 deletions smoke-tests/test/proxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,22 @@ const setup = require('./fixtures/setup.js')

t.test('proxy', async t => {
const { npm, readFile } = await setup(t, {
testdir: {
project: {
'.npmrc': '',
'package.json': {
name: 'placeholder',
},
},
},
mockRegistry: false,
useProxy: true,
})

await npm('install', '[email protected]')

t.strictSame(await readFile('package.json'), {
name: 'placeholder',
dependencies: { abbrev: '^1.0.4' },
})
})
42 changes: 42 additions & 0 deletions test/lib/commands/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
const t = require('tap')
const { load: loadMockNpm } = require('../../fixtures/mock-npm')
const fs = require('node:fs')
const path = require('node:path')

t.test('exec commands', async t => {
await t.test('with args, dev=true', async t => {
Expand All @@ -9,6 +11,9 @@ t.test('exec commands', async t => {
let ARB_OBJ = null

const { npm } = await loadMockNpm(t, {
prefixDir: {
'package.json': '{ "name": "does not matter" }',
},
mocks: {
'@npmcli/run-script': ({ event }) => {
SCRIPTS.push(event)
Expand Down Expand Up @@ -53,6 +58,9 @@ t.test('exec commands', async t => {
let ARB_OBJ = null

const { npm } = await loadMockNpm(t, {
prefixDir: {
'package.json': '{ "name": "does not matter" }',
},
mocks: {
'@npmcli/run-script': ({ event }) => {
SCRIPTS.push(event)
Expand Down Expand Up @@ -93,6 +101,9 @@ t.test('exec commands', async t => {
const SCRIPTS = []
let REIFY_CALLED = false
const { npm } = await loadMockNpm(t, {
prefixDir: {
'package.json': '{ "name": "does not matter" }',
},
mocks: {
'{LIB}/utils/reify-finish.js': async () => {},
'@npmcli/run-script': ({ event }) => {
Expand Down Expand Up @@ -165,6 +176,37 @@ t.test('exec commands', async t => {
)
})

await t.test('should throw when package.json does not exist', async t => {
const { npm, prefix } = await loadMockNpm(t, {})
await t.rejects(
npm.exec('install'),
/ENOENT: no such file or directory/,
'should throw before reify'
)
t.notOk(
fs.existsSync(path.join(prefix, './package-lock.json')),
'should not generate package-lock.json'
)
})

await t.test('should throw when package.json is an empty object', async t => {
const { npm, prefix } = await loadMockNpm(t, {
prefixDir: {
'package.json': '{}',
},
})
await t.rejects(
npm.exec('install'),
/EEMPTYPKGJSON: package.json is empty object/,
'should throw before reify'
)

t.notOk(
fs.existsSync(path.join(prefix, './package-lock.json')),
'should not generate package-lock.json'
)
})

await t.test('npm i -g npm engines check success', async t => {
const { npm } = await loadMockNpm(t, {
mocks: {
Expand Down
2 changes: 1 addition & 1 deletion test/lib/utils/audit-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const auditError = async (t, { command, error, ...config } = {}) => {
command,
config,
exec: true,
prefixDir: { 'package.json': '{}', 'package-lock.json': '{}' },
prefixDir: { 'package.json': '{ "name": "does not matter" }', 'package-lock.json': '{}' },
})

const res = {}
Expand Down