Skip to content

refactor: use WHATWG URL instead of url.parse#48674

Merged
jkleinsc merged 1 commit intomainfrom
remove-url-parse
Mar 9, 2026
Merged

refactor: use WHATWG URL instead of url.parse#48674
jkleinsc merged 1 commit intomainfrom
remove-url-parse

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented Oct 27, 2025 •
edited by nikwen
Loading

Description of Change

Fixes #49550
Fixes #49840

Node is deprecating url.parse:

(node:34012) [DEP0169] DeprecationWarning: url.parse() behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for url.parse() vulnerabilities.

Checklist

Release Notes

Notes: none

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/39-x-y PR should also be added to the "39-x-y" branch. labels Oct 27, 2025
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Oct 27, 2025
@codebytere codebytere force-pushed the remove-url-parse branch 2 times, most recently from aeefb54 to c0d8f9f Compare October 27, 2025 12:26
clavin
clavin approved these changes Oct 27, 2025
View reviewed changes
Copy link
Member

@clavin clavin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love to see it 👍

lib/common/api/net-client-request.ts
@@ -227,8 +227,7 @@ function validateHeader (name: any, value: any): void {
}

function parseOptions (optionsIn: ClientRequestConstructorOptions | string): NodeJS.CreateURLLoaderOptions & ExtraURLLoaderOptions {
Copy link
Member

@clavin clavin Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: this function feels like it needs a gentle rewrite overall. (non-blocking since the changes here preserve existing behavior 👍)

ckerr
ckerr requested changes Oct 27, 2025
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the deprecated code LGTM.

Marking as "request changes" because of the question about main.ts. Before merging this PR, it would be good to get info on why we had to revert this change the last time we did it.

default_app/main.ts Outdated
const file = option.file;
// eslint-disable-next-line n/no-deprecated-api
const protocol = url.parse(file).protocol;
const protocol = new URL(file).protocol;
Copy link
Member

@ckerr ckerr Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

I'm suspicious of this change because Sam tried doing this before and then backed it out.

Do we ever accept weird URLs here where url.parse() and WHATWG might differ, e.g. passing in a Windows file path with backslashes?

@MarshallOfSound (or anyone else) DYK why that change was backed out?

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Oct 28, 2025
@github-actions github-actions bot added the target/40-x-y PR should also be added to the "40-x-y" branch. label Oct 28, 2025
@electron electron deleted a comment Nov 3, 2025
@ckerr
Copy link
Member

ckerr commented Nov 25, 2025

In general this PR is a Good Thing and I feel kinda bad that I've played a part in its staying in limbo for a month.

@MarshallOfSound reping on #48674 (comment)

In the alternate, @codebytere WDYT about using WHATURL everywhere except that one line that's in contention?

@codebytere
Copy link
Member Author

codebytere commented Nov 25, 2025

@ckerr done!

@codebytere codebytere requested a review from ckerr November 25, 2025 17:22
@github-actions github-actions bot added the target/41-x-y PR should also be added to the "41-x-y" branch. label Jan 19, 2026
jkleinsc
jkleinsc requested changes Jan 21, 2026
View reviewed changes
Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the change causes tests to fail.

@codebytere codebytere mentioned this pull request Jan 29, 2026
Closed
5 tasks
jkleinsc
jkleinsc requested changes Feb 3, 2026
View reviewed changes
Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like tests are still failing.

@nikwen nikwen mentioned this pull request Feb 18, 2026
Closed
3 tasks
@jkleinsc jkleinsc merged commit eacec9a into main Mar 9, 2026
62 checks passed
@jkleinsc jkleinsc deleted the remove-url-parse branch March 9, 2026 15:12
@welcome
Copy link

welcome bot commented Mar 9, 2026

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk
Copy link

release-clerk bot commented Mar 9, 2026

No Release Notes

@trop
Copy link
Contributor

trop bot commented Mar 9, 2026

I was unable to backport this PR to "39-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/39-x-y and removed target/39-x-y PR should also be added to the "39-x-y" branch. labels Mar 9, 2026
This was referenced Mar 9, 2026
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Mar 9, 2026

I have automatically backported this PR to "40-x-y", please check out #50142

@trop
Copy link
Contributor

trop bot commented Mar 9, 2026

I have automatically backported this PR to "41-x-y", please check out #50143

@trop trop bot added in-flight/40-x-y in-flight/41-x-y and removed target/40-x-y PR should also be added to the "40-x-y" branch. target/41-x-y PR should also be added to the "41-x-y" branch. labels Mar 9, 2026
@jkleinsc jkleinsc mentioned this pull request Mar 9, 2026
Closed
4 tasks
@trop trop bot added merged/40-x-y PR was merged to the "40-x-y" branch. merged/41-x-y PR was merged to the "41-x-y" branch. and removed in-flight/40-x-y in-flight/41-x-y labels Mar 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ckerr ckerr ckerr approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

@clavin clavin clavin approved these changes

@samuelmaddock samuelmaddock Awaiting requested review from samuelmaddock

Assignees

No one assigned

Labels

merged/40-x-y PR was merged to the "40-x-y" branch. merged/41-x-y PR was merged to the "41-x-y" branch. needs-manual-bp/39-x-y semver/patch backwards-compatible bug fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Usage of electron.net.request triggers url.parse deprecation warning Usage of url.parse in default app triggers deprecation warning

4 participants

@codebytere @ckerr @jkleinsc @clavin