Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

fix: socket variable testing for undefined - #1726

#1726
Merged
jimmywarting merged 4 commits into
node-fetch:2.xnode-fetch/node-fetch:2.xfrom
joamag:2.xCopy head branch name to clipboard
Jun 29, 2023
Merged

fix: socket variable testing for undefined#1726
jimmywarting merged 4 commits into
node-fetch:2.xnode-fetch/node-fetch:2.xfrom
joamag:2.xCopy head branch name to clipboard

Conversation

@joamag

@joamag joamag commented Mar 17, 2023

Copy link
Copy Markdown

Avoid issue where socket event was not triggered and local socket variable is not defined. This issue is reproducible only using deno.

Purpose

#1725

Changes

Adds undefined testing to socket variable, protecting it from TypeError.


Avoid issue where socket event was not triggered and local socket vraible is not defined.
This issue is reproducible only using deno.
@joamag

joamag commented Mar 25, 2023

Copy link
Copy Markdown
Author

@jimmywarting do you think you can merge this one? It's a simple undefined validation 🙏

@node-fetch node-fetch deleted a comment from joamag May 13, 2023
@jimmywarting

Copy link
Copy Markdown
Collaborator
	request.on('response', response => {
		const {headers} = response;
		if (headers['transfer-encoding'] === 'chunked' && !headers['content-length']) {
			response.once('close', hadError => {
				// if for some reason the 'socket' event was not triggered
				// for the request (happens in deno) avoids `TypeError`
				if (socket === undefined) {
					return;
				}

				// if a data listener is still present we didn't end cleanly
				const hasDataListener = socket.listenerCount('data') > 0;

				if (hasDataListener && !hadError) {
					const err = new Error('Premature close');
					err.code = 'ERR_STREAM_PREMATURE_CLOSE';
					errorCallback(err);
				}
			});
		}
	});

don't we still want to handle close callback at what is at the bottom?
aka:

				if (hasDataListener && !hadError) {
					const err = new Error('Premature close');
					err.code = 'ERR_STREAM_PREMATURE_CLOSE';
					errorCallback(err);
				}

maybe would be better to just do:

				// if for some reason the 'socket' event was not triggered
				// for the request (happens in deno) avoids `TypeError`
				if (socket) {
				    // if a data listener is still present we didn't end cleanly
				    const hasDataListener = socket.listenerCount('data') > 0;
				}

Only controls the execution of the section that uses socket.
@joamag

joamag commented May 14, 2023

Copy link
Copy Markdown
Author
	request.on('response', response => {
		const {headers} = response;
		if (headers['transfer-encoding'] === 'chunked' && !headers['content-length']) {
			response.once('close', hadError => {
				// if for some reason the 'socket' event was not triggered
				// for the request (happens in deno) avoids `TypeError`
				if (socket === undefined) {
					return;
				}

				// if a data listener is still present we didn't end cleanly
				const hasDataListener = socket.listenerCount('data') > 0;

				if (hasDataListener && !hadError) {
					const err = new Error('Premature close');
					err.code = 'ERR_STREAM_PREMATURE_CLOSE';
					errorCallback(err);
				}
			});
		}
	});

don't we still want to handle close callback at what is at the bottom? aka:

				if (hasDataListener && !hadError) {
					const err = new Error('Premature close');
					err.code = 'ERR_STREAM_PREMATURE_CLOSE';
					errorCallback(err);
				}

maybe would be better to just do:

				// if for some reason the 'socket' event was not triggered
				// for the request (happens in deno) avoids `TypeError`
				if (socket) {
				    // if a data listener is still present we didn't end cleanly
				    const hasDataListener = socket.listenerCount('data') > 0;
				}

Yes it makes more sense.
It's done 👍

Comment thread src/index.js Outdated
Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>
@joamag

joamag commented May 21, 2023

Copy link
Copy Markdown
Author

@jimmywarting should be ready for merge 😀

@joamag

joamag commented Jun 4, 2023

Copy link
Copy Markdown
Author

@LinusU Can you the this PR, it would be super useful to have this one merged :) 🙏

@jimmywarting
jimmywarting enabled auto-merge (squash) June 29, 2023 19:15
@jimmywarting
jimmywarting merged commit 8bc3a7c into node-fetch:2.x Jun 29, 2023
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 2.6.12 🎉

The release is available on:

Your semantic-release bot 📦🚀

@joamag
joamag deleted the 2.x branch July 6, 2023 07:40
joamag added a commit to hivesolutions/yonius that referenced this pull request Oct 25, 2023
Including node-fetch as it should now be compatible with deno.
According to node-fetch/node-fetch#1726.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.