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: allow hash character in paths #5122

Merged
merged 3 commits into from Jul 27, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 7 additions & 7 deletions lib/commands/diff.js
Expand Up @@ -106,7 +106,7 @@ class Diff extends BaseCommand {
const pkgName = await this.packageName(this.prefix)
return [
`${pkgName}@${this.npm.config.get('tag')}`,
`file:${this.prefix}`,
`file:${this.prefix.replace(/#/g, '%23')}`,
]
}

Expand Down Expand Up @@ -134,7 +134,7 @@ class Diff extends BaseCommand {
}
return [
`${pkgName}@${a}`,
`file:${this.prefix}`,
`file:${this.prefix.replace(/#/g, '%23')}`,
]
}

Expand Down Expand Up @@ -165,7 +165,7 @@ class Diff extends BaseCommand {
}
return [
`${spec.name}@${spec.fetchSpec}`,
`file:${this.prefix}`,
`file:${this.prefix.replace(/#/g, '%23')}`,
]
}

Expand All @@ -178,7 +178,7 @@ class Diff extends BaseCommand {
}
}

const aSpec = `file:${node.realpath}`
const aSpec = `file:${node.realpath.replace(/#/g, '%23')}`

// finds what version of the package to compare against, if a exact
// version or tag was passed than it should use that, otherwise
Expand Down Expand Up @@ -211,8 +211,8 @@ class Diff extends BaseCommand {
]
} else if (spec.type === 'directory') {
return [
`file:${spec.fetchSpec}`,
`file:${this.prefix}`,
`file:${spec.fetchSpec.replace(/#/g, '%23')}`,
`file:${this.prefix.replace(/#/g, '%23')}`,
]
} else {
throw this.usageError(`Spec type ${spec.type} not supported.`)
Expand Down Expand Up @@ -279,7 +279,7 @@ class Diff extends BaseCommand {

const res = !node || !node.package || !node.package.version
? spec.fetchSpec
: `file:${node.realpath}`
: `file:${node.realpath.replace(/#/g, '%23')}`

return `${spec.name}@${res}`
})
Expand Down
4 changes: 2 additions & 2 deletions lib/commands/link.js
Expand Up @@ -122,7 +122,7 @@ class Link extends ArboristWorkspaceCmd {
...this.npm.flatOptions,
prune: false,
path: this.npm.prefix,
add: names.map(l => `file:${resolve(globalTop, 'node_modules', l)}`),
add: names.map(l => `file:${resolve(globalTop, 'node_modules', l).replace(/#/g, '%23')}`),
save,
workspaces: this.workspaceNames,
})
Expand All @@ -133,7 +133,7 @@ class Link extends ArboristWorkspaceCmd {
async linkPkg () {
const wsp = this.workspacePaths
const paths = wsp && wsp.length ? wsp : [this.npm.prefix]
const add = paths.map(path => `file:${path}`)
const add = paths.map(path => `file:${path.replace(/#/g, '%23')}`)
const globalTop = resolve(this.npm.globalDir, '..')
const arb = new Arborist({
...this.npm.flatOptions,
Expand Down
5 changes: 5 additions & 0 deletions tap-snapshots/test/lib/commands/link.js.test.cjs
Expand Up @@ -5,6 +5,11 @@
* Make sure to inspect the output below. Do not ignore changes!
*/
'use strict'
exports[`test/lib/commands/link.js TAP hash character in working directory path > should create a global link to current pkg, even within path with hash 1`] = `
{CWD}/test/lib/commands/tap-testdir-link-hash-character-in-working-directory-path/global-prefix/lib/node_modules/test-pkg-link -> {CWD}/test/lib/commands/tap-testdir-link-hash-character-in-working-directory-path/i_like_#_in_my_paths/test-pkg-link

`

exports[`test/lib/commands/link.js TAP link global linked pkg to local nm when using args > should create a local symlink to global pkg 1`] = `
{CWD}/test/lib/commands/tap-testdir-link-link-global-linked-pkg-to-local-nm-when-using-args/my-project/node_modules/@myscope/bar -> {CWD}/test/lib/commands/tap-testdir-link-link-global-linked-pkg-to-local-nm-when-using-args/global-prefix/lib/node_modules/@myscope/bar
{CWD}/test/lib/commands/tap-testdir-link-link-global-linked-pkg-to-local-nm-when-using-args/my-project/node_modules/@myscope/linked -> {CWD}/test/lib/commands/tap-testdir-link-link-global-linked-pkg-to-local-nm-when-using-args/scoped-linked
Expand Down
36 changes: 36 additions & 0 deletions test/lib/commands/link.js
Expand Up @@ -514,3 +514,39 @@ t.test('--global option', async t => {
'should throw an useful error'
)
})

t.test('hash character in working directory path', async t => {
const testdir = t.testdir({
'global-prefix': {
lib: {
node_modules: {
a: {
'package.json': JSON.stringify({
name: 'a',
version: '1.0.0',
}),
},
},
},
},
'i_like_#_in_my_paths': {
'test-pkg-link': {
'package.json': JSON.stringify({
name: 'test-pkg-link',
version: '1.0.0',
}),
},
},
})
npm.globalDir = resolve(testdir, 'global-prefix', 'lib', 'node_modules')
npm.prefix = resolve(testdir, 'i_like_#_in_my_paths', 'test-pkg-link')

link.workspacePaths = null
AgainPsychoX marked this conversation as resolved.
Show resolved Hide resolved
await link.exec([]);
const links = await printLinks({
path: resolve(npm.globalDir, '..'),
global: true,
})

t.matchSnapshot(links, 'should create a global link to current pkg, even within path with hash')
})
4 changes: 2 additions & 2 deletions workspaces/arborist/lib/arborist/build-ideal-tree.js
Expand Up @@ -484,7 +484,7 @@ Try using the package name instead, e.g:
.catch(/* istanbul ignore next */ er => null)
if (st && st.isSymbolicLink()) {
const target = await readlink(dir)
const real = resolve(dirname(dir), target)
const real = resolve(dirname(dir), target).replace(/#/g, '%23')
tree.package.dependencies[name] = `file:${real}`
} else {
tree.package.dependencies[name] = '*'
Expand Down Expand Up @@ -603,7 +603,7 @@ Try using the package name instead, e.g:
if (filepath) {
const { name } = spec
const tree = this.idealTree.target
spec = npa(`file:${relpath(tree.path, filepath)}`, tree.path)
spec = npa(`file:${relpath(tree.path, filepath).replace(/#/g, '%23')}`, tree.path)
spec.name = name
}
return spec
Expand Down
2 changes: 1 addition & 1 deletion workspaces/arborist/lib/arborist/load-actual.js
Expand Up @@ -196,7 +196,7 @@ module.exports = cls => class ActualLoader extends cls {
const actualRoot = tree.isLink ? tree.target : tree
const { dependencies = {} } = actualRoot.package
for (const [name, kid] of actualRoot.children.entries()) {
const def = kid.isLink ? `file:${kid.realpath}` : '*'
const def = kid.isLink ? `file:${kid.realpath.replace(/#/g, '%23')}` : '*'
dependencies[name] = dependencies[name] || def
}
actualRoot.package = { ...actualRoot.package, dependencies }
Expand Down
2 changes: 1 addition & 1 deletion workspaces/arborist/lib/arborist/load-virtual.js
Expand Up @@ -162,7 +162,7 @@ module.exports = cls => class VirtualLoader extends cls {
lockfile: s.data,
})
for (const [name, path] of workspaces.entries()) {
lockWS.push(['workspace', name, `file:${path}`])
lockWS.push(['workspace', name, `file:${path.replace(/#/g, '%23')}`])
}

const lockEdges = [
Expand Down
2 changes: 1 addition & 1 deletion workspaces/arborist/lib/arborist/reify.js
Expand Up @@ -1240,7 +1240,7 @@ module.exports = cls => class Reifier extends cls {
// path initially, in which case we can end up with the wrong
// thing, so just get the ultimate fetchSpec and relativize it.
const p = req.fetchSpec.replace(/^file:/, '')
const rel = relpath(addTree.realpath, p)
const rel = relpath(addTree.realpath, p).replace(/#/g, '%23')
newSpec = `file:${rel}`
}
} else {
Expand Down
4 changes: 2 additions & 2 deletions workspaces/arborist/lib/consistent-resolve.js
Expand Up @@ -20,8 +20,8 @@ const consistentResolve = (resolved, fromPath, toPath, relPaths = false) => {
raw,
} = npa(resolved, fromPath)
const isPath = type === 'file' || type === 'directory'
return isPath && !relPaths ? `file:${fetchSpec}`
: isPath ? 'file:' + (toPath ? relpath(toPath, fetchSpec) : fetchSpec)
return isPath && !relPaths ? `file:${fetchSpec.replace(/#/g, '%23')}`
: isPath ? 'file:' + (toPath ? relpath(toPath, fetchSpec.replace(/#/g, '%23')) : fetchSpec.replace(/#/g, '%23'))
: hosted ? `git+${
hosted.auth ? hosted.https(hostedOpt) : hosted.sshurl(hostedOpt)
}`
Expand Down
2 changes: 1 addition & 1 deletion workspaces/arborist/lib/link.js
Expand Up @@ -118,7 +118,7 @@ class Link extends Node {
// the path/realpath guard is there for the benefit of setting
// these things in the "wrong" order
return this.path && this.realpath
? `file:${relpath(dirname(this.path), this.realpath)}`
? `file:${relpath(dirname(this.path), this.realpath).replace(/#/g, '%23')}`
: null
}

Expand Down
2 changes: 1 addition & 1 deletion workspaces/arborist/lib/node.js
Expand Up @@ -824,7 +824,7 @@ class Node {
}

for (const [name, path] of this[_workspaces].entries()) {
new Edge({ from: this, name, spec: `file:${path}`, type: 'workspace' })
new Edge({ from: this, name, spec: `file:${path.replace(/#/g, '%23')}`, type: 'workspace' })
}
}

Expand Down
6 changes: 3 additions & 3 deletions workspaces/arborist/lib/shrinkwrap.js
Expand Up @@ -815,7 +815,7 @@ class Shrinkwrap {
const pathFixed = !resolved ? null
: !/^file:/.test(resolved) ? resolved
// resolve onto the metadata path
: `file:${resolve(this.path, resolved.slice(5))}`
: `file:${resolve(this.path, resolved.slice(5)).replace(/#/g, '%23')}`

// if we have one, only set the other if it matches
// otherwise it could be for a completely different thing.
Expand Down Expand Up @@ -996,7 +996,7 @@ class Shrinkwrap {
: npa.resolve(node.name, edge.spec, edge.from.realpath)

if (node.isLink) {
lock.version = `file:${relpath(this.path, node.realpath)}`
lock.version = `file:${relpath(this.path, node.realpath).replace(/#/g, '%23')}`
} else if (spec && (spec.type === 'file' || spec.type === 'remote')) {
lock.version = spec.saveSpec
} else if (spec && spec.type === 'git' || rSpec.type === 'git') {
Expand Down Expand Up @@ -1074,7 +1074,7 @@ class Shrinkwrap {
// this especially shows up with workspace edges when the root
// node is also a workspace in the set.
const p = resolve(node.realpath, spec.slice('file:'.length))
set[k] = `file:${relpath(node.realpath, p)}`
set[k] = `file:${relpath(node.realpath, p).replace(/#/g, '%23')}`
} else {
set[k] = spec
}
Expand Down