Skip to content

feat(node/http): implement ClientRequest.setTimeout() #18783

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

Merged
merged 4 commits into from
Apr 22, 2023

Conversation

levex
Copy link
Contributor

@levex levex commented Apr 20, 2023

  • implement setTimeout with matching semantics of Node
  • add the test from Node but leave it turned off because ClientRequest has no underlying socket

@levex levex requested a review from bartlomieju April 20, 2023 14:22
@bartlomieju bartlomieju requested a review from kt3k April 20, 2023 17:07
@kt3k
Copy link
Member

kt3k commented Apr 21, 2023

Thanks for the PR! The implementation looks good to me as first pass.

Could you drop the file test-http-client-set-timeout.js from the commit. The files under cli/tests/node_compat/test/ will be automatically synchronized with cli/tests/node_compat/config.json by executing tools/node_compat/setup.ts. So that file will be removed when someone called setup.ts

@levex levex force-pushed the node-clientrequest-settimeout branch from d512e1d to f3ab90e Compare April 21, 2023 17:36
@levex
Copy link
Contributor Author

levex commented Apr 21, 2023

@kt3k Done!

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

2 participants