-
Notifications
You must be signed in to change notification settings - Fork 1
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
fixup: simplify #12
base: transform-by
Are you sure you want to change the base?
fixup: simplify #12
Conversation
ec7f248
to
b20d8ca
Compare
test/parallel/test-transform-by.js
Outdated
transformByFuncReturnsObjectWithoutSymbolAsyncIterator(), | ||
transformByEncoding(), | ||
// NOTE: This doesn't make sense for iterable? Is it consistent with Readable.from? | ||
// transformByEncoding(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this test, tests for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in order to have an analogue to (chunk, enc, cb) => { }
we add an encoding
property for cases where transform streams behave differently depending on the encoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is taken into account in Readable.from
and since we are doing objectMode
I'm still unsure what the semantics here should be. I need to think about this a bit more unless someone can clarify?
941e354
to
30bc1ad
Compare
c4235a4
to
134db8b
Compare
134db8b
to
62d6326
Compare
} | ||
|
||
Promise.all([ | ||
transformBy(), | ||
transformByFuncReturnsObjectWithSymbolAsyncIterator(), | ||
transformByObjReturnedWSymbolAsyncIteratorWithNonPromiseReturningNext(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidmarkclements: I don't think this test is needed. Returning a non promise is fine?
iterator = iterable[SymbolIterator](); | ||
else | ||
|
||
if (!iterator || typeof iterator.next !== 'function') | ||
throw new ERR_INVALID_ARG_TYPE('iterable', ['Iterable'], iterable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidmarkclements Moved invalid arg check to Readable.from
.
@davidmarkclements I've updated to include type checks |
@davidmarkclements: I don't understand the encoding test or implementation. Where else in streams related code do you see anything like this? Do you think you can explain it's purpose? |
5bf4f6e
to
4bfffea
Compare
Tries to simplify nodejs#28501