Skip to content

https: make https.globalAgent overridable also under ECMAScript Modules#48808

Closed
whut wants to merge 1 commit intonodejs:mainfrom
whut:pr-https-globalAgent
Closed

https: make https.globalAgent overridable also under ECMAScript Modules#48808
whut wants to merge 1 commit intonodejs:mainfrom
whut:pr-https-globalAgent

Conversation

@whut
Copy link

@whut whut commented Jul 17, 2023

Under ECMAScript modules when you do "import * as https from 'https'" you get a new object with properties copied from https module exports. So if this is a regular data property, you will just override a copy, but if this would be a accessor property, we can still access the actual https.globalAgent.

Refs: #25170, #9386

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 17, 2023

Review requested:

  • @nodejs/crypto
  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added https Issues or PRs related to the https subsystem. needs-ci PRs that need a full CI run. labels Jul 17, 2023
mcollina
mcollina requested changes Jul 17, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Thanks for opening a PR! Can you please add a unit test?

@whut whut force-pushed the pr-https-globalAgent branch from 4b4dba5 to 54579fb Compare July 17, 2023 08:58
@whut
Copy link
Author

whut commented Jul 17, 2023

@mcollina test added, based on test-https-client-override-global-agent.js

@whut whut requested a review from mcollina July 17, 2023 09:01
mcollina
mcollina approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 17, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 17, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/52806/

@targos
Copy link
Member

targos commented Jul 17, 2023

Does it already work correctly with http.globalAgent ?

@whut whut force-pushed the pr-https-globalAgent branch from 54579fb to a0dfaa5 Compare July 17, 2023 09:20
@whut
Copy link
Author

whut commented Jul 17, 2023

Does it already work correctly with http.globalAgent ?

Yes, because it already use accessor property

@whut whut force-pushed the pr-https-globalAgent branch from a0dfaa5 to 161b003 Compare July 17, 2023 09:26
@github-actions github-actions bot mentioned this pull request Jul 18, 2023
Open
26 tasks
@whut
Copy link
Author

whut commented Jul 18, 2023

@mcollina I fixed the build failure, I sadly missed to add one "import" of primordial

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2023
@mcollina
Copy link
Member

mcollina commented Jul 18, 2023

The linter is failing.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 18, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/52829/

ShogunPanda
ShogunPanda approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions github-actions bot mentioned this pull request Jul 19, 2023
Open
25 tasks
@whut whut force-pushed the pr-https-globalAgent branch from 161b003 to c8c19cd Compare July 19, 2023 08:07
@github-actions github-actions bot mentioned this pull request Jul 20, 2023
Open
22 tasks
@whut
https: make https.globalAgent overridable also under ECMAScript Modules
c1381e0 
Under ECMAScript modules when you do "import * as https from 'https'"
you get a new object with properties copied from https module exports.
So if this is a regular data property, you will just override a copy,
but if this would be a accessor property, we can still access the actual
https.globalAgent.

Refs: nodejs#25170, nodejs#9386
@whut whut force-pushed the pr-https-globalAgent branch from c8c19cd to c1381e0 Compare July 20, 2023 10:13
@whut
Copy link
Author

whut commented Jul 20, 2023

@mcollina I only now run the test locally, and fixed imports in it to work under ECMAScript modules (e.g. ... from '../common/' to ... from '../common/index.mjs'. Test now pass, and linter is also happy:)

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 20, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/52868/

@github-actions github-actions bot mentioned this pull request Jul 21, 2023
Open
16 tasks
@mcollina
Copy link
Member

mcollina commented Jul 21, 2023

CI failed https://github.com/nodejs/node/actions/runs/5609712188/job/15200153620?pr=48808#step:6:5524

This was referenced Jul 22, 2023
Open
Open
Open
Open
Open
Open
Open
@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label May 11, 2024
@github-actions
Copy link
Contributor

github-actions bot commented May 11, 2024

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@ShogunPanda
Copy link
Contributor

ShogunPanda commented May 15, 2024

@whut Any interest on delivering this? Can you please rebase this PR after the last main branch so we can run CI again?

@avivkeller avivkeller closed this Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

Assignees

No one assigned

Labels

https Issues or PRs related to the https subsystem. needs-ci PRs that need a full CI run. stalled Issues and PRs that are stalled.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@whut @nodejs-github-bot @targos @mcollina @ShogunPanda @marco-ippolito @aduh95 @avivkeller