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.

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: warn on invalid publishConfig #8078

Merged
merged 1 commit into from
Feb 3, 2025
Merged
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
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/test-package@1.0.0 prepublishOnly
> touch scripts-prepublishonly
> @npmcli/test-package@1.0.0 publish
> touch scripts-publish
> @npmcli/test-package@1.0.0 postpublish
> touch scripts-postpublish
+ @npmcli/test-package@1.0.0
`

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.'])
wraithgar marked this conversation as resolved.
Show resolved Hide resolved
})

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