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

[BUG] v12.0.2 breaks npm test "hash character in working directory path" #203

Closed
1 task done
TrevorBurnham opened this issue Feb 15, 2025 · 2 comments
Closed
1 task done
Labels
Bug thing that needs fixing Needs Triage needs an initial review

Comments

@TrevorBurnham
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

I noticed something surprising while submitted a patch to npm/cli (npm/cli#8108): On the current latest branch (3a80a7b7), if I run node ./scripts/resetdeps.js in accordance with the Contributing docs, npm-package-arg gets updated from 12.0.1 to 12.0.2. After that change, the test "hash character in working directory path" fails:

 FAIL ​ test/lib/commands/link.js 1 failed of 15 3s
 ✖Could not read package.json: Error: ENOENT: no such file or directory, open '/code/npm-cli/test/lib/commands/tap-testdir-link-hash
  -character-in-working-directory-path/other/i_like_%23_in_my_paths/test-pkg-link/package.json'

Expected Behavior

As a contributor to npm/cli, I would expect all tests to pass after I pull latest and run node ./scripts/resetdeps.js.

Steps To Reproduce

git clone https://github.com/npm/cli.git
cd cli
node ./scripts/resetdeps.js
node . run test test/lib/commands/link.js

Environment

As the failing CI tests for npm/cli#8108 show, this issue is happening across all environments.

@TrevorBurnham TrevorBurnham added Bug thing that needs fixing Needs Triage needs an initial review labels Feb 15, 2025
TrevorBurnham added a commit to TrevorBurnham/npm-package-arg that referenced this issue Feb 16, 2025
TrevorBurnham added a commit to TrevorBurnham/cli that referenced this issue Feb 17, 2025
This commit updates npm-package-arg from v12.0.1 to v12.0.2. It also
removes all usage of `.replace(/#/g, '%23')` for compatibility with the
new version.

See:
npm/npm-package-arg#203
@TrevorBurnham
Copy link
Author

I see two options here:

  1. Find a way to retain backward compatibility, by satisfying the test case at test: Add test case for regression in npm/cli (#203) #204.
  2. Require downstream packages to stop escaping hash characters (and possibly others) before passing file paths to npm-package-arg, as in deps: npm-package-arg@12.0.2 cli#8112.

Since the changes in #200 went out in a patch release, I think finding a way to follow it up with another patch for backward compatibility would be ideal. Though either way, removing the escaping code in npm/cli feels like a nice bit of code cleanup. cc @wraithgar @reggi

@wraithgar
Copy link
Member

As of right now we're considering this a test regression. The path forward is for us to make a PR updating just that dep and fix the test, and determine if it's something npm needs to handle or if the test was just working around buggy behavior.

wraithgar pushed a commit to npm/cli that referenced this issue Feb 18, 2025
Removes all usage of `.replace(/#/g, '%23')` for compatibility with the new version

This is the code changes from #8112 isloated from the dependency update itself.

Closes:
npm/npm-package-arg#203

Credit: @TrevorBurnham
wraithgar pushed a commit to npm/cli that referenced this issue Feb 18, 2025
Removes all usage of `.replace(/#/g, '%23')` for compatibility with the new version

This is the code changes from #8112 isloated from the dependency update itself.

Closes:
npm/npm-package-arg#203

Credit: @TrevorBurnham
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs an initial review
Projects
None yet
Development

No branches or pull requests

2 participants