Skip to content

feat: [net] add "priority" option to net.request#42628

Merged
jkleinsc merged 1 commit intoelectron:mainfrom
zeeker999:net-request-priority
May 30, 2025
Merged

feat: [net] add "priority" option to net.request#42628
jkleinsc merged 1 commit intoelectron:mainfrom
zeeker999:net-request-priority

Conversation

@zeeker999
Copy link
Copy Markdown
Contributor

@zeeker999 zeeker999 commented Jun 23, 2024 •
edited by jkleinsc
Loading

Description of Change

Add the priority and priorityIncremental options to customize the Priority header of requests sent by net.request.
Currently all requests sent by net.request have a priority header with value u=4, i, this maybe unexpected. We want it fully customizable and match the Chrome exactly if possible.

Close #46680

Checklist

Release Notes

Notes: Added the priority and priorityIncremental options to net.request()

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jun 23, 2024
@zeeker999 zeeker999 force-pushed the net-request-priority branch 4 times, most recently from 80f8e4f to f86a56c Compare June 23, 2024 06:50
@zeeker999 zeeker999 marked this pull request as draft June 23, 2024 23:00
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Jun 30, 2024
@zeeker999 zeeker999 force-pushed the net-request-priority branch 2 times, most recently from 5835726 to bdaaf91 Compare April 17, 2025 14:46
@zeeker999 zeeker999 marked this pull request as ready for review April 17, 2025 14:55
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Apr 17, 2025
@codebytere codebytere added semver/minor backwards-compatible functionality api-review/requested 🗳 labels Apr 18, 2025
@codebytere codebytere requested a review from a team April 18, 2025 18:41
@zeeker999 zeeker999 force-pushed the net-request-priority branch from bdaaf91 to bb7f80b Compare April 18, 2025 21:41
nornagon
nornagon reviewed Apr 18, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

I'm no longer officially a maintainer, but I wrote a lot of this code so here's a review

lib/common/api/net-client-request.ts
Comment on lines +294 to +296
if ('priorityIncremental' in options) {
urlLoaderOptions.priorityIncremental = options.priorityIncremental;
}
Copy link
Copy Markdown
Contributor

@nornagon nornagon Apr 18, 2025

Choose a reason for hiding this comment

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

any reason not to have this as a key in the object above unconditionally?

Copy link
Copy Markdown
Contributor Author

@zeeker999 zeeker999 Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To preserve the default value(true), otherwise the priorityIncremental will be false if it's unspecified and then assigned to undefined unconditionally.

@zeeker999
Copy link
Copy Markdown
Contributor Author

zeeker999 commented Apr 19, 2025 •
edited
Loading

I just go ahead and moved the test out.

@zeeker999 zeeker999 force-pushed the net-request-priority branch from 9a2b8a9 to b11e4d8 Compare April 19, 2025 00:11
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Apr 24, 2025
erickzhao
erickzhao approved these changes May 1, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

API LGTM

@zeeker999
Copy link
Copy Markdown
Contributor Author

zeeker999 commented May 20, 2025

All Build Electron jobs were failed to due to the 360 minutes limit. Is there anything that I can do to fix this and should I rebase my branch to the latest main branch?

@zeeker999 zeeker999 force-pushed the net-request-priority branch from 10623b9 to 042eada Compare May 20, 2025 07:54
@zeeker999 @jkleinsc
feat: [net] add "priority" option to net.request
c47d67b 
document the default value of priority option

Update the priority test to not use the httpbin.org as server

Fixed the lint errors

Fixed the build error
@jkleinsc jkleinsc force-pushed the net-request-priority branch from 042eada to c47d67b Compare May 21, 2025 23:11
itsananderson
itsananderson reviewed May 22, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@itsananderson itsananderson left a comment

Choose a reason for hiding this comment

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

API LGTM

@zeeker999
Copy link
Copy Markdown
Contributor Author

zeeker999 commented May 22, 2025 •
edited
Loading

Could you add the backport labels so this PR can be backported to other branches like 37-x-y and 36-x-y.

@clavin clavin added target/36-x-y PR should also be added to the "36-x-y" branch. target/37-x-y PR should also be added to the "37-x-y" branch. and removed no-backport labels May 28, 2025
@nikwen

This comment was marked as outdated.

Sign in to view
@trop

This comment was marked as outdated.

Sign in to view
@jkleinsc jkleinsc merged commit dc5efca into electron:main May 30, 2025
69 of 85 checks passed
@release-clerk
Copy link
Copy Markdown

release-clerk bot commented May 30, 2025

Release Notes Persisted

Added the priority and priorityIncremental options to net.request()

This was referenced May 30, 2025
Merged
Merged
@trop
Copy link
Copy Markdown
Contributor

trop bot commented May 30, 2025

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

@trop
Copy link
Copy Markdown
Contributor

trop bot commented May 30, 2025

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

@trop trop bot added in-flight/37-x-y and removed target/36-x-y PR should also be added to the "36-x-y" branch. target/37-x-y PR should also be added to the "37-x-y" branch. labels May 30, 2025
@trop trop bot added merged/36-x-y PR was merged to the "36-x-y" branch. merged/37-x-y PR was merged to the "37-x-y" branch. and removed in-flight/36-x-y in-flight/37-x-y labels Jun 8, 2025
kigh-ota pushed a commit to kigh-ota/electron that referenced this pull request Sep 30, 2025
@zeeker999 @kigh-ota
feat: [net] add "priority" option to net.request (electron#42628)
578e512 
document the default value of priority option

Update the priority test to not use the httpbin.org as server

Fixed the lint errors

Fixed the build error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@nornagon nornagon Awaiting requested review from nornagon

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

+2 more reviewers

@itsananderson itsananderson itsananderson left review comments

@erickzhao erickzhao erickzhao approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api-review/approved ✅ merged/36-x-y PR was merged to the "36-x-y" branch. merged/37-x-y PR was merged to the "37-x-y" branch. semver/minor backwards-compatible functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The priority header is hardcoded for requests sent by net.request

9 participants

@zeeker999 @nikwen @nornagon @itsananderson @jkleinsc @MarshallOfSound @erickzhao @codebytere @clavin