Skip to content

Commit 0cba981

Browse files
zkochanzkochan
committed
refactor: address Copilot review feedback
* Relax named-registry parser to accept unscoped names (work:lodash@^4) and fall back to any (scoped or unscoped) outer alias for <prefix>:<range>. Drops the validateScopedPackageName helper; scoped-name validation stays inline for the @-prefixed branch. * Optimize replaceVersionInBareSpecifier hot path: check 'npm:' first then for-loop over namedRegistryPrefixes, avoiding a per-call array allocation. * Guard replaceEnvInStringValues against arrays so a YAML list under registries:/namedRegistries: surfaces as a config error instead of being silently coerced to {0: ..., 1: ...}.
1 parent f53e776 commit 0cba981

5 files changed

Lines changed: 72 additions & 37 deletions

File tree

‎config/reader/src/getOptionsFromRootManifest.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ function replaceEnvInSettings (settings: PnpmSettings): PnpmSettings {
6666
}
6767

6868
function replaceEnvInStringValues (value: unknown): unknown {
69-
if (value == null || typeof value !== 'object') return value
69+
if (value == null || typeof value !== 'object' || Array.isArray(value)) return value
7070
const out: Record<string, unknown> = {}
7171
for (const [k, v] of Object.entries(value as Record<string, unknown>)) {
7272
out[k] = typeof v === 'string' ? envReplace(v, process.env) : v

‎installing/deps-resolver/src/replaceVersionInBareSpecifier.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,17 @@ export function replaceVersionInBareSpecifier (
88
if (semver.validRange(bareSpecifier)) {
99
return version
1010
}
11-
const prefix = ['npm:', ...namedRegistryPrefixes].find((p) => bareSpecifier.startsWith(p))
11+
let prefix: string | undefined
12+
if (bareSpecifier.startsWith('npm:')) {
13+
prefix = 'npm:'
14+
} else {
15+
for (const candidate of namedRegistryPrefixes) {
16+
if (bareSpecifier.startsWith(candidate)) {
17+
prefix = candidate
18+
break
19+
}
20+
}
21+
}
1222
if (prefix == null) {
1323
return bareSpecifier
1424
}

‎resolving/npm-resolver/src/parseBareSpecifier.ts‎

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,9 @@ export interface NamedRegistryPackageSpec extends RegistryPackageSpec {
9595
// Parses a named-registry specifier of the shape `<alias>:<body>` into a
9696
// RegistryPackageSpec. Returns `null` when the specifier does not use one of
9797
// the configured aliases, so the caller can fall through to other resolvers.
98-
// Supported shapes (mirrors the `gh:` prefix, which is the canonical example):
99-
// - `<alias>:@<owner>/<name>[@<version_selector>]`
100-
// - `<alias>:<version_selector>` when paired with a scoped package alias
98+
// Supported shapes:
99+
// - `<alias>:[@<owner>/]<name>[@<version_selector>]`
100+
// - `<alias>:<version_selector>` paired with a package alias
101101
export function parseNamedRegistrySpecifierToRegistryPackageSpec (
102102
rawSpecifier: string,
103103
knownAliases: ReadonlySet<string>,
@@ -113,27 +113,45 @@ export function parseNamedRegistrySpecifierToRegistryPackageSpec (
113113
let pkgName: string
114114
let versionSelector: string | undefined
115115

116-
if (body[0] === '@') {
117-
// syntax: <alias>:@<owner>/<name>[@<version_selector>]
116+
if (semver.validRange(body) != null) {
117+
// `<alias>:<version_selector>` — fall back to the dependency alias as
118+
// the package name. Unresolvable without one.
119+
if (!packageAlias) return null
120+
pkgName = packageAlias
121+
versionSelector = body
122+
} else if (body[0] === '@') {
123+
// `<alias>:@<owner>/<name>[@<version_selector>]` — scoped package.
118124
const index = body.lastIndexOf('@')
119125
if (index === 0) {
120126
pkgName = body
121127
} else {
122128
pkgName = body.substring(0, index)
123129
versionSelector = body.substring(index + '@'.length)
124130
}
131+
if (pkgName.indexOf('/') === -1 || pkgName.endsWith('/')) {
132+
throw new PnpmError(
133+
'INVALID_NAMED_REGISTRY_PACKAGE_NAME',
134+
`The package name '${pkgName}' in named registry '${registryName}:' is invalid`
135+
)
136+
}
125137
} else if (packageAlias?.startsWith('@')) {
126-
// syntax: <alias>:<version_selector> paired with a scoped package alias
138+
// `<alias>:<tag>` paired with a scoped alias — body is a version
139+
// selector (tag/dist-tag). Mirrors GitHub Packages, where the package
140+
// is always scoped and a bare body is a tag.
127141
pkgName = packageAlias
128142
versionSelector = body
129143
} else {
130-
// No scoped alias means we cannot know the package name — let other
131-
// resolvers try.
132-
return null
144+
// `<alias>:<name>[@<version_selector>]` — unscoped package in body.
145+
const index = body.lastIndexOf('@')
146+
if (index < 1) {
147+
pkgName = body
148+
} else {
149+
pkgName = body.substring(0, index)
150+
versionSelector = body.substring(index + '@'.length)
151+
}
152+
if (!pkgName) return null
133153
}
134154

135-
validateScopedPackageName(pkgName, registryName)
136-
137155
const selector = getVersionSelectorType(versionSelector ?? defaultTag)
138156
if (selector == null) return null
139157

@@ -144,19 +162,3 @@ export function parseNamedRegistrySpecifierToRegistryPackageSpec (
144162
registryName,
145163
}
146164
}
147-
148-
function validateScopedPackageName (pkgName: string, registryName: string): void {
149-
if (pkgName[0] !== '@') {
150-
throw new PnpmError(
151-
'MISSING_NAMED_REGISTRY_PACKAGE_SCOPE',
152-
`Package '${pkgName}' from named registry '${registryName}:' must have a scope (e.g. '@owner/name')`
153-
)
154-
}
155-
const sepIndex = pkgName.indexOf('/')
156-
if (sepIndex === -1 || sepIndex === pkgName.length - 1) {
157-
throw new PnpmError(
158-
'INVALID_NAMED_REGISTRY_PACKAGE_NAME',
159-
`The package name '${pkgName}' in named registry '${registryName}:' is invalid`
160-
)
161-
}
162-
}

‎resolving/npm-resolver/test/parseBareSpecifier.test.ts‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -115,15 +115,38 @@ describe('parseNamedRegistrySpecifierToRegistryPackageSpec', () => {
115115
}))
116116
})
117117

118-
test('does not claim <alias>:<version_selector> when no scoped alias is provided', () => {
118+
test('does not claim <alias>:<version_selector> when no package alias is provided', () => {
119119
// No alias means we cannot know the package name — we must not hijack such specifiers.
120120
expect(parseNamedRegistrySpecifierToRegistryPackageSpec('gh:^1.0.0', GH_ALIASES, undefined, DEFAULT_TAG)).toBeNull()
121121
})
122122

123-
test('does not claim <alias>:<version_selector> when the alias is not scoped', () => {
124-
// GitHub Packages names are always scoped. If the alias isn't, the bare specifier is
125-
// probably meant for another resolver.
126-
expect(parseNamedRegistrySpecifierToRegistryPackageSpec('gh:^1.0.0', GH_ALIASES, 'foo', DEFAULT_TAG)).toBeNull()
123+
test('falls back to the dependency alias for <alias>:<version_selector>, scoped or not', () => {
124+
// Mirrors the `npm:^1.0.0` shape — works with both scoped and unscoped aliases.
125+
expect(parseNamedRegistrySpecifierToRegistryPackageSpec('gh:^1.0.0', GH_ALIASES, '@acme/foo', DEFAULT_TAG)).toMatchObject({
126+
name: '@acme/foo',
127+
registryName: 'gh',
128+
})
129+
expect(parseNamedRegistrySpecifierToRegistryPackageSpec('work:^4.0.0', new Set(['work']), 'lodash', DEFAULT_TAG)).toMatchObject({
130+
name: 'lodash',
131+
registryName: 'work',
132+
})
133+
})
134+
135+
test('parses unscoped <alias>:<name>[@<version_selector>]', () => {
136+
// Arbitrary named registries (vlt-style) accept unscoped names too,
137+
// not just GitHub Packages-style scopes.
138+
expect(parseNamedRegistrySpecifierToRegistryPackageSpec('work:lodash@^4.0.0', new Set(['work']), undefined, DEFAULT_TAG)).toMatchObject({
139+
name: 'lodash',
140+
fetchSpec: '>=4.0.0 <5.0.0-0',
141+
type: 'range',
142+
registryName: 'work',
143+
})
144+
expect(parseNamedRegistrySpecifierToRegistryPackageSpec('work:lodash', new Set(['work']), undefined, DEFAULT_TAG)).toMatchObject({
145+
name: 'lodash',
146+
fetchSpec: 'latest',
147+
type: 'tag',
148+
registryName: 'work',
149+
})
127150
})
128151

129152
test('matches any alias in the configured set and reports it back to the caller', () => {

‎resolving/npm-resolver/test/resolveNamedRegistry.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -244,15 +244,15 @@ test('resolveFromNamedRegistry() does not claim the github: git shortcut scheme'
244244
await expect(resolveFromNamedRegistry({ bareSpecifier: 'github:@acme/foo' }, {})).resolves.toBeNull()
245245
})
246246

247-
test('resolveFromNamedRegistry() returns null when the alias is not scoped (unambiguous inputs are left for other resolvers)', async () => {
247+
test('resolveFromNamedRegistry() returns null when no alias is provided for a bare version selector', async () => {
248248
const { resolveFromNamedRegistry } = createNpmResolver(fetch, () => undefined, {
249249
storeDir: temporaryDirectory(),
250250
cacheDir: temporaryDirectory(),
251251
registries,
252252
})
253253

254-
// Without a scoped package alias, `gh:<version>` cannot be resolved to a GitHub Packages name.
255-
await expect(resolveFromNamedRegistry({ alias: 'private', bareSpecifier: 'gh:2.0.0' }, {})).resolves.toBeNull()
254+
// Without any package alias, `gh:<version>` cannot map to a package name.
255+
await expect(resolveFromNamedRegistry({ bareSpecifier: 'gh:2.0.0' }, {})).resolves.toBeNull()
256256
})
257257

258258
test('resolveFromNamedRegistry() throws when the specifier names an invalid scoped package', async () => {

0 commit comments

Comments
 (0)