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

Handle streams attached to an error by replacing them with '[object Stream]' #57

Merged
merged 3 commits into from Feb 14, 2022

Conversation

wodka
Copy link
Contributor

@wodka wodka commented Jan 30, 2022

fixes #56

image

@Richienb
Copy link
Contributor

If the module is going to handle readables, then what about writables and transform streams?

@Richienb
Copy link
Contributor

Richienb commented Jan 31, 2022

Also, this can be used: https://github.com/sindresorhus/is-stream

@wodka
Copy link
Contributor Author

wodka commented Jan 31, 2022

Also, this can be used: https://github.com/sindresorhus/is-stream

would make sense - what do you think about adding this as a dependency?

@sindresorhus
Copy link
Owner

This needs a test.

@sindresorhus
Copy link
Owner

would make sense - what do you think about adding this as a dependency?

Nah. I think what you have is fine for now. https://github.com/sindresorhus/serialize-error/pull/57/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R80

@sindresorhus
Copy link
Owner

sindresorhus commented Feb 6, 2022

This also needs to support https://nodejs.org/api/webstreams.html#class-readablestream (web streams)

@fregante
Copy link
Contributor

Same applies here: #55 (comment)

I didn't realize that serialize-error already includes Buffer accommodations. Even that one seems unrelated.

@wodka
Copy link
Contributor Author

wodka commented Feb 13, 2022

update: working on the tests

@sindresorhus
Copy link
Owner

I didn't realize that serialize-error already includes Buffer accommodations. Even that one seems unrelated.

It's just to make the common use-case nicer. It's not meant for two-way transfer. It's mostly just to make it nicer for logging and storing errors.

@wodka
Copy link
Contributor Author

wodka commented Feb 13, 2022

@sindresorhus would you think it makes more sense to split it up to show a text depending on the stream type? Something along the lines of this:

		if (typeof value === 'object' && typeof value.pipe === 'function') {
			const writeable = typeof value._write === 'function' && typeof value._writableState === 'object';
			const readable = typeof value._read === 'function' && typeof value._readableState === 'object';

			if (writeable && readable && typeof value._transform === 'function') {
				to[key] = '[object TransformStream]';
				continue;
			}

			if (writeable && readable) {
				to[key] = '[object DuplexStream]';
				continue;
			}

			if (readable) {
				to[key] = '[object ReadableStream]';
				continue;
			}

			if (writeable) {
				to[key] = '[object WriteableStream]';
				continue;
			}

			to[key] = '[object Stream]';
			continue;
		}

@sindresorhus
Copy link
Owner

would you think it makes more sense to split it up to show a text depending on the stream type? Something along the lines of this:

No, let's keep it simple.

@wodka
Copy link
Contributor Author

wodka commented Feb 13, 2022

ok, then I think it's done

@sindresorhus sindresorhus changed the title show [object Readable] if there is a readable object Handle streams attached to an error by replacing them with '[object Stream]' Feb 14, 2022
@sindresorhus sindresorhus merged commit b589f8e into sindresorhus:main Feb 14, 2022
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.

AWS SDK v3 exposes Readable in Error object
4 participants