stream: remove TODO and add a description instead #27086
Conversation
After looking into this it turned out that these two errors are sanity checks that should not be reached. It is unfortunate that we assigned error codes for these but changing it into an assertion seems to be a hassle for `readable-streams`.
nodejs-github-bot
added
the
stream
BridgeAR
added
the
author ready
lib/_stream_transform.js
Outdated
| // 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(internal/assert has nothing to do with assert anymore. They are 100% independent)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, I struggle what to do here in this case. @Trott are you fine with this comment due to the issues with readable-streams?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Another alternative would be to remove these checks completely.
@mcollina are you fine with that?
|
Landed in 57fd70f 🎉 |
After looking into this it turned out that these two errors are sanity checks that should not be reached. It is unfortunate that we assigned error codes for these but changing it into an assertion seems to be a hassle for `readable-streams`. PR-URL: nodejs#27086 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
After looking into this it turned out that these two errors are sanity checks that should not be reached. It is unfortunate that we assigned error codes for these but changing it into an assertion seems to be a hassle for `readable-streams`. PR-URL: #27086 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
After looking into this it turned out that these two errors are
sanity checks that should not be reached. It is unfortunate that
we assigned error codes for these but changing it into an assertion
seems to be a hassle for
readable-streams.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes