Skip to content
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.

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

lookup: add https-proxy-agent #752

Merged
merged 1 commit into from Apr 22, 2022
Merged

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Oct 9, 2019

This package has ~6 million weekly downloads and we broke it in
Node.js 10.0.0

Refs: nodejs/node#24474 (comment)

Checklist
  • npm test passes
  • contribution guidelines followed
    here

@codecov-io
Copy link

codecov-io commented Oct 9, 2019

Codecov Report

Merging #752 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
- Coverage   96.22%   96.17%   -0.05%     
==========================================
  Files          27       30       +3     
  Lines         874      889      +15     
==========================================
+ Hits          841      855      +14     
- Misses         33       34       +1     
Impacted Files Coverage Δ
lib/lookup.js 96.15% <0.00%> (-1.22%) ⬇️
bin/citgm-all.js 92.00% <0.00%> (-0.16%) ⬇️
bin/citgm.js 100.00% <0.00%> (ø)
lib/spawn.js 100.00% <0.00%> (ø)
lib/unpack.js 100.00% <0.00%> (ø)
lib/timeout.js 100.00% <0.00%> (ø)
lib/git-clone.js 100.00% <0.00%> (ø)
lib/common-args.js 100.00% <0.00%> (ø)
lib/create-options.js 100.00% <0.00%> (ø)
lib/temp-directory.js 100.00% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27e267f...89a71f3. Read the comment docs.

@targos
Copy link
Member

targos commented Oct 15, 2019

@lpinca
Copy link
Member Author

lpinca commented May 15, 2020

@nodejs/citgm ping

@targos
Copy link
Member

targos commented May 16, 2020

This fails on all supported versions of Node.js:

@lpinca
Copy link
Member Author

lpinca commented May 16, 2020

Interesting. It seems that a build step (npm run build) is required before running tests. There is also a test failure due to recent stream changes.

These issues should be fixed before this can land.

@targos
Copy link
Member

targos commented May 16, 2020

if there's a build script to run, you can specify it in a "scripts" property.
Example:

"scripts": ["rebuild-tests", "test"],

@lpinca
Copy link
Member Author

lpinca commented May 16, 2020

Ok, thanks Michaël. Trying to figure how to fix the failing tests now (this is why I want this in CITGM, we broke this module again).

@lpinca
Copy link
Member Author

lpinca commented May 16, 2020

It should now fail only on Node.js 14 (TooTallNate/proxy-agents#104).

@MylesBorins MylesBorins changed the base branch from master to main August 14, 2020 21:00
@targos
Copy link
Member

targos commented Mar 5, 2021

Is it possible to cut a release so the module is fixed on Node.js 14 and 15, and we can merge this?

@lpinca
Copy link
Member Author

lpinca commented Mar 5, 2021

See TooTallNate/proxy-agents#104 (comment). If maintainers are not responsive I think it does not make sense to add this.

@targos
Copy link
Member

targos commented Mar 7, 2021

Is the module entirely broken on Node.js 14+ without the patch from TooTallNate/proxy-agents#104 ?

@lpinca
Copy link
Member Author

lpinca commented Mar 9, 2021

Not completely broken, only if the proxy replies with a non 200 status code.

@lpinca
Copy link
Member Author

lpinca commented Apr 21, 2022

https-proxy-agent@5.0.1 has been released. This should be finally ready to land.

@targos
Copy link
Member

targos commented Apr 21, 2022

could you please rebase so we test the PR with the latest code?

This package has ~6 million monthly downloads and we broke it in
Node.js 10.0.0

Refs: nodejs/node#24474 (comment)
@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2022

Codecov Report

Merging #752 (d3acc07) into main (fcfc781) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #752   +/-   ##
=======================================
  Coverage   96.45%   96.45%           
=======================================
  Files          28       28           
  Lines        2145     2145           
=======================================
  Hits         2069     2069           
  Misses         76       76           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcfc781...d3acc07. Read the comment docs.

@targos
Copy link
Member

targos commented Apr 21, 2022

@targos
Copy link
Member

targos commented Apr 22, 2022

CI looks good to me. The only failure is due to Ubuntu 18.04 which cannot run node 18.

@targos targos merged commit 7089166 into nodejs:main Apr 22, 2022
@lpinca lpinca deleted the add/https-proxy-agent branch April 22, 2022 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants