Skip to content

url: update encodePathChars to handle special characters properly #54516

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

url: update encodePathChars to handle special characters properly #54516

wants to merge 1 commit into from

Conversation

injunchoi98
Copy link
Contributor

Fixes #54515

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Aug 23, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 23, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.34%. Comparing base (e70bd47) to head (d73af15).
Report is 16 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #54516   +/-   ##
=======================================
  Coverage   87.34%   87.34%           
=======================================
  Files         649      649           
  Lines      182524   182533    +9     
  Branches    35026    35033    +7     
=======================================
+ Hits       159420   159430   +10     
+ Misses      16373    16372    -1     
  Partials     6731     6731
Files Coverage Δ
lib/internal/url.js 94.93% <100.00%> (+0.02%) ⬆️

... and 30 files with indirect coverage changes

@aduh95
Copy link
Contributor

aduh95 commented Aug 23, 2024 •
edited
Loading

I'm not sure it's worth changing, in what scenario those char can be problematic in the path? EDIT: the answer is in #54515 (comment)

@aduh95 aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label Aug 23, 2024
@aduh95 aduh95 mentioned this pull request Aug 24, 2024
Merged
@avivkeller
Copy link
Member

Can this be closed in favor of @aduh95's PR?

@injunchoi98
Copy link
Contributor Author

Of course! Thank you for let me know ☺️

@injunchoi98 injunchoi98 deleted the issue-54505 branch August 25, 2024 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pathToFileURL function in url fails to handle special characters properly

4 participants

@injunchoi98 @nodejs-github-bot @aduh95 @avivkeller