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

Conversation

AgainPsychoX
Copy link
Contributor

@AgainPsychoX AgainPsychoX commented Jul 3, 2022

Allow hash character in paths

Instead file:${...} we need to use pathToFileURL (from node:url) because some paths can contain hash character (#) or other characters (I guess, question mark (?) could also be a problem) that are cut off when the URI (file: schema) points file to be accessed by file system.

The file:${...} instances would need replacing hash character (#) with its URI encoded value (%23) to prevent it being reparsed along the way. My previous idea of using pathToFileURL wouldn't work, as relative paths got pre-resolved before converting to URL, i.e. file:../hello with CWD of S:\\test\\directory would became file://S:\\test\\hello while some instances of code (NPA) expected not unresolved file:..\\hello. Aside of that, simply replacing the char would be faster. By the way, I am almost certain that the hash character is the only "weird" character that could be used in the paths, as question mark (despite being beginning of query params) hardly can't nor should be used for names (Windows disallows).

References

Fixes #5120

Pending work

  • Check other places where file: was used and if similar patch could be applied.

@AgainPsychoX AgainPsychoX force-pushed the againpsychox/allow-hash-in-paths branch from 813615b to 6b4da91 Compare July 4, 2022 00:37
@AgainPsychoX AgainPsychoX marked this pull request as ready for review July 4, 2022 01:06
@AgainPsychoX AgainPsychoX requested a review from a team as a code owner July 4, 2022 01:06
@darcyclarke darcyclarke added the semver:patch semver patch level for changes label Jul 26, 2022
@lukekarrys
Copy link
Member

Looks good, thanks for the fix!

It's possible in the future we may want a standardized way of doing this. We've had mixed luck with small utils to do stuff like this, so this is great for now. Just commenting in case we want to DRY it up in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm link fails when running inside path with hash tag #
4 participants