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

test: Add test case for regression in npm/cli (#203) #204

Closed
wants to merge 1 commit into from

Conversation

TrevorBurnham
Copy link

I looked into #203. What's happening is that Arborist is providing npa with a path where the # character has already been URL-encoded, like this:

npa('file:/test_%23dir')

Prior to #200, the result looked like this:

{
  type: 'directory',
  name: null,
  rawSpec: 'file:/test_%23dir',
  fetchSpec: '/test_#dir',
  saveSpec: 'file:/test_#dir',
}

After #200, the result looks like this:

{
  type: 'directory',
  name: null,
  rawSpec: 'file:/test_%23dir',
  fetchSpec: '/test_%23dir',
  saveSpec: 'file:/test_%23dir',
}

I'm not sure what the best fix for this is, but I hope this (failing) test case helps. I believe the key change is around this line:

resolvedUrl = new URL(rawSpec, `${pathToFileURL(path.resolve(where))}/`)

cc @wraithgar @reggi

@wraithgar
Copy link
Member

This is a tricky one. I think supporting this is the wrong direction. This was a very deep-seated bug that took quite a bit of research to figure out, even going in and pinging the maintainers of Node.js itself who were well versed in the ecma spec on these things. The connection between a file: url, a path, and other kinds of urls is ... pretty complicated. URI encoding doesn't overlap with file urls, and there are also special functions like url.pathToFileURL.

At the end of the day, making the tool do the right thing was the bugfix. It should require valid input. This should have been fixed here originally instead of worked around in npm itself.

I'm going to tentatively close this based on the fact that npm/cli#8115 exists.

@wraithgar wraithgar closed this Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants