Skip to content

stream: remove TODO and add a description instead #27086

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.

Sign up for GitHub

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

Closed
BridgeAR wants to merge 2 commits into from
Closed

stream: remove TODO and add a description instead #27086

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
739dc8e
stream: remove TODO and add a description instead
BridgeAR Apr 4, 2019
6091cea
fixup! stream: remove TODO and add a description instead
BridgeAR May 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions lib/_stream_transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,7 @@ function done(stream, er, data) {
if (data != null) // Single equals check for both `null` and `undefined`
stream.push(data);

// TODO(BridgeAR): Write a test for these two error cases
// if there's nothing in the write buffer, then that means
// that nothing more will ever be provided
// These two error cases are sanity checks that can likely not be tested.
Copy link
Copy Markdown
Member

@Trott Trott Apr 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Does this mean that the errors should be impossible in theory? Or simply difficult to test? If impossible, let's change them to assert() instead?

Copy link
Copy Markdown
Member

@mcollina mcollina Apr 5, 2019

Choose a reason for hiding this comment

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

We should not depend on assert() in readable-stream (bundle size, etc), and I would need to spend time to actually remove it with regexps. So, can we avoid adding it in the first place?

Copy link
Copy Markdown
Member

@Trott Trott Apr 5, 2019

Choose a reason for hiding this comment

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

We should not depend on assert() in readable-stream (bundle size, etc), and I would need to spend time to actually remove it with regexps. So, can we avoid adding it in the first place?

Yeah, I wasn't meaning lib/assert.js but rather the tiny (15 LoC) lib/internal/assert.js (that conditionally loads lib/assert.js only if it is actually throwing). But I guess that doesn't help readable-stream?

Copy link
Copy Markdown
Member

@mcollina mcollina Apr 5, 2019

Choose a reason for hiding this comment

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

not at all :/.

Copy link
Copy Markdown
Member

@Trott Trott Apr 5, 2019

Choose a reason for hiding this comment

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

OK, in that case, how about just clearing up the comment to indicate whether this is something that is believed to logically be impossible (that is: we believe this error should never happen) or if it's just something that will indeed happen from time to time (due to bugs in people's code or problems on a host or whatever) and is merely difficult/impossible to reliably test.

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Apr 27, 2019

Choose a reason for hiding this comment

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

(internal/assert has nothing to do with assert anymore. They are 100% independent)

Copy link
Copy Markdown
Member

@mcollina mcollina Apr 27, 2019

Choose a reason for hiding this comment

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

I highly prefer not to, because it will make it harder to track future changes to that file as well. Adding another require could lead to 1-3 days of added work, and I prefer to be conservative for what is currently ok.

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Apr 29, 2019

Choose a reason for hiding this comment

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

Ok, I struggle what to do here in this case. @Trott are you fine with this comment due to the issues with readable-streams?

Copy link
Copy Markdown
Member

@Trott Trott Apr 29, 2019

Choose a reason for hiding this comment

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

Ok, I struggle what to do here in this case. @Trott are you fine with this comment due to the issues with readable-streams?

Yeah, consider my comments to be nits. Would love to have consensus on something that makes everyone happy and addresses my comments/questions, but ultimately, I'm fine with whatever you and Matteo can agree on.

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR May 1, 2019

Choose a reason for hiding this comment

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

Another alternative would be to remove these checks completely.
@mcollina are you fine with that?

Comment thread
BridgeAR marked this conversation as resolved.
Outdated
if (stream._writableState.length)
throw new ERR_TRANSFORM_WITH_LENGTH_0();

Expand Down