Skip to content

Commit

Permalink
fix: warn on invalid publishConfig (#8078)
Browse files Browse the repository at this point in the history
  • Loading branch information
wraithgar authored Feb 3, 2025
1 parent 41417de commit 879303c
Show file tree
Hide file tree
Showing 9 changed files with 47 additions and 9 deletions.
3 changes: 3 additions & 0 deletions lib/commands/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,9 @@ ${defData}
const { content } = await pkgJson.normalize(this.npm.prefix).catch(() => ({ content: {} }))

if (content.publishConfig) {
for (const key in content.publishConfig) {
this.npm.config.checkUnknown('publishConfig', key)
}
const pkgPath = resolve(this.npm.prefix, 'package.json')
msg.push(`; "publishConfig" from ${pkgPath}`)
msg.push('; This set of config values will be used at publish-time.', '')
Expand Down
5 changes: 5 additions & 0 deletions lib/commands/publish.js
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,11 @@ class Publish extends BaseCommand {
// corresponding `publishConfig` settings
const filteredPublishConfig = Object.fromEntries(
Object.entries(manifest.publishConfig).filter(([key]) => !(key in cliFlags)))
if (logWarnings) {
for (const key in filteredPublishConfig) {
this.npm.config.checkUnknown('publishConfig', key)
}
}
flatten(filteredPublishConfig, opts)
}
return manifest
Expand Down
3 changes: 3 additions & 0 deletions lib/commands/unpublish.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ class Unpublish extends BaseCommand {
// corresponding `publishConfig` settings
const filteredPublishConfig = Object.fromEntries(
Object.entries(manifest.publishConfig).filter(([key]) => !(key in cliFlags)))
for (const key in filteredPublishConfig) {
this.npm.config.checkUnknown('publishConfig', key)
}
flatten(filteredPublishConfig, opts)
}

Expand Down
9 changes: 8 additions & 1 deletion tap-snapshots/test/lib/commands/config.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,13 @@ color = {COLOR}
; "publishConfig" from {CWD}/prefix/package.json
; This set of config values will be used at publish-time.
_authToken = (protected)
//some.registry:_authToken = (protected)
other = "not defined"
registry = "https://some.registry"
`

exports[`test/lib/commands/config.js TAP config list with publishConfig local > warns about unknown config 1`] = `
Array [
"Unknown publishConfig config /"other/". This will stop working in the next major version of npm.",
]
`
9 changes: 9 additions & 0 deletions tap-snapshots/test/lib/commands/publish.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,15 @@ exports[`test/lib/commands/publish.js TAP re-loads publishConfig.registry if add

exports[`test/lib/commands/publish.js TAP respects publishConfig.registry, runs appropriate scripts > new package version 1`] = `
> @npmcli/[email protected] prepublishOnly
> touch scripts-prepublishonly
> @npmcli/[email protected] publish
> touch scripts-publish
> @npmcli/[email protected] postpublish
> touch scripts-postpublish
+ @npmcli/[email protected]
`

exports[`test/lib/commands/publish.js TAP restricted access > must match snapshot 1`] = `
Expand Down
6 changes: 4 additions & 2 deletions test/lib/commands/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,16 +164,17 @@ t.test('config list with publishConfig', async t => {
prefixDir: {
'package.json': JSON.stringify({
publishConfig: {
other: 'not defined',
registry: 'https://some.registry',
_authToken: 'mytoken',
'//some.registry:_authToken': 'mytoken',
},
}),
},
...opts,
})

t.test('local', async t => {
const { npm, joinedOutput } = await loadMockNpmWithPublishConfig(t)
const { npm, logs, joinedOutput } = await loadMockNpmWithPublishConfig(t)

await npm.exec('config', ['list'])

Expand All @@ -182,6 +183,7 @@ t.test('config list with publishConfig', async t => {
t.match(output, 'registry = "https://some.registry"')

t.matchSnapshot(output, 'output matches snapshot')
t.matchSnapshot(logs.warn, 'warns about unknown config')
})

t.test('global', async t => {
Expand Down
10 changes: 7 additions & 3 deletions test/lib/commands/publish.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,14 @@ t.test('respects publishConfig.registry, runs appropriate scripts', async t => {
publish: 'touch scripts-publish',
postpublish: 'touch scripts-postpublish',
},
publishConfig: { registry: alternateRegistry },
publishConfig: {
other: 'not defined',
registry: alternateRegistry,
},
}
const { npm, joinedOutput, prefix, registry } = await loadNpmWithRegistry(t, {
const { npm, joinedOutput, logs, prefix, registry } = await loadNpmWithRegistry(t, {
config: {
loglevel: 'silent',
loglevel: 'warn',
[`${alternateRegistry.slice(6)}/:_authToken`]: 'test-other-token',
},
prefixDir: {
Expand All @@ -49,6 +52,7 @@ t.test('respects publishConfig.registry, runs appropriate scripts', async t => {
t.equal(fs.existsSync(path.join(prefix, 'scripts-prepublish')), false, 'did not run prepublish')
t.equal(fs.existsSync(path.join(prefix, 'scripts-publish')), true, 'ran publish')
t.equal(fs.existsSync(path.join(prefix, 'scripts-postpublish')), true, 'ran postpublish')
t.same(logs.warn, ['Unknown publishConfig config "other". This will stop working in the next major version of npm.'])
})

t.test('re-loads publishConfig.registry if added during script process', async t => {
Expand Down
7 changes: 6 additions & 1 deletion test/lib/commands/unpublish.js
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ t.test('dryRun with no args', async t => {

t.test('publishConfig no spec', async t => {
const alternateRegistry = 'https://other.registry.npmjs.org'
const { joinedOutput, npm } = await loadMockNpm(t, {
const { logs, joinedOutput, npm } = await loadMockNpm(t, {
config: {
force: true,
'//other.registry.npmjs.org/:_authToken': 'test-other-token',
Expand All @@ -390,6 +390,7 @@ t.test('publishConfig no spec', async t => {
name: pkg,
version: '1.0.0',
publishConfig: {
other: 'not defined',
registry: alternateRegistry,
},
}, null, 2),
Expand All @@ -406,6 +407,10 @@ t.test('publishConfig no spec', async t => {
registry.unpublish({ manifest })
await npm.exec('unpublish', [])
t.equal(joinedOutput(), '- test-package')
t.same(logs.warn, [
'using --force Recommended protections disabled.',
'Unknown publishConfig config "other". This will stop working in the next major version of npm.',
])
})

t.test('prioritize CLI flags over publishConfig no spec', async t => {
Expand Down
4 changes: 2 additions & 2 deletions workspaces/config/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -584,14 +584,14 @@ class Config {
}
// Some defaults like npm-version are not user-definable and thus don't have definitions
if (where !== 'default') {
this.#checkUnknown(where, key)
this.checkUnknown(where, key)
}
conf.data[k] = v
}
}
}

#checkUnknown (where, key) {
checkUnknown (where, key) {
if (!this.definitions[key]) {
if (internalEnv.includes(key)) {
return
Expand Down

0 comments on commit 879303c

Please sign in to comment.