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

fix: socket variable testing for undefined #1726

Merged
merged 4 commits into from
Jun 29, 2023
Merged

fix: socket variable testing for undefined #1726

merged 4 commits into from
Jun 29, 2023

Conversation

joamag
Copy link

@joamag joamag commented Mar 17, 2023

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
Copy link
Author

joamag commented Mar 25, 2023

@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
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
Copy link
Author

joamag commented May 14, 2023

	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 👍

src/index.js Outdated Show resolved Hide resolved
Co-authored-by: Linus Unnebäck <[email protected]>
@joamag
Copy link
Author

joamag commented May 21, 2023

@jimmywarting should be ready for merge 😀

@joamag
Copy link
Author

joamag commented Jun 4, 2023

@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

🎉 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