Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add example for request + "Garbage Collection" #3916

Merged
merged 5 commits into from
Dec 8, 2024

Conversation

WTCT-TOP
Copy link
Contributor

@WTCT-TOP WTCT-TOP commented Dec 2, 2024

This relates to...

See #3914

Changes

Added an example for request + "Garbage Collection".

Status

README.md Outdated Show resolved Hide resolved
@KhafraDev
Copy link
Member

It was added under the fetch specification compliance section. It should be moved elsewhere.

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.

Can you remove the unnecessary promises?

@WTCT-TOP
Copy link
Contributor Author

WTCT-TOP commented Dec 3, 2024

@KhafraDev do you think I should move the whole GC section or just the new example for request?
@mcollina what do you mean by "unnecessary promises"? use async/await instead of .then/.catch? I followed the existing example..

Also, is there a reason to await res.body.dump() if I don't care about the body? isn't res.body.dump() (without await) good enough?

@mcollina
Copy link
Member

mcollina commented Dec 7, 2024

@mcollina what do you mean by "unnecessary promises"? use async/await instead of .then/.catch?

yes exactly. Currently this allocates one promise too much ;).

Also, is there a reason to await res.body.dump() if I don't care about the body? isn't res.body.dump() (without await) good enough?

If you don't await dump, you'll still have asynchronous activity happening in the background, as the body will not be dumped immediately. Not awaiting it defeats the purpose of the example, as without it there will be lingering memory and activity running.

@WTCT-TOP
Copy link
Contributor Author

WTCT-TOP commented Dec 7, 2024

I've updated both examples to use async await, let me know what you think..

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 merged commit 03edf68 into nodejs:main Dec 8, 2024
18 of 31 checks passed
This was referenced Dec 16, 2024
ToddyTheNoobDud added a commit to ToddyTheNoobDud/AquaLink that referenced this pull request Dec 18, 2024
- This feature was implemented in undici 7.1.1, according to nodejs/undici#3916 (comment)
ToddyTheNoobDud added a commit to ToddyTheNoobDud/AquaLink that referenced this pull request Dec 18, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants