Skip to content

Comments

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

Open
codebytere wants to merge 1 commit intomainfrom
remove-url-parse
Open

refactor: use WHATWG URL instead of url.parse#48674
codebytere wants to merge 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
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkleinsc jkleinsc jkleinsc requested changes

@ckerr ckerr ckerr approved these changes

@clavin clavin clavin approved these changes

@samuelmaddock samuelmaddock Awaiting requested review from samuelmaddock

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

semver/patch backwards-compatible bug fixes target/39-x-y PR should also be added to the "39-x-y" branch. 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.

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