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

fixup: simplify #12

Open
wants to merge 6 commits into
base: transform-by
Choose a base branch
from
Open

fixup: simplify #12

wants to merge 6 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 16, 2019

Tries to simplify nodejs#28501

transformByFuncReturnsObjectWithoutSymbolAsyncIterator(),
transformByEncoding(),
// NOTE: This doesn't make sense for iterable? Is it consistent with Readable.from?
// transformByEncoding(),
Copy link
Member Author

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?

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.

Copy link
Member Author

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?

@ronag ronag force-pushed the transform-by-2 branch 4 times, most recently from 941e354 to 30bc1ad Compare December 16, 2019 22:16
@ronag ronag force-pushed the transform-by-2 branch 7 times, most recently from c4235a4 to 134db8b Compare December 16, 2019 23:18
}

Promise.all([
transformBy(),
transformByFuncReturnsObjectWithSymbolAsyncIterator(),
transformByObjReturnedWSymbolAsyncIteratorWithNonPromiseReturningNext(),
Copy link
Member Author

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);
Copy link
Member Author

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.

@ronag
Copy link
Member Author

ronag commented Dec 17, 2019

@davidmarkclements I've updated to include type checks

@ronag
Copy link
Member Author

ronag commented Dec 17, 2019

@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?

@ronag ronag force-pushed the transform-by-2 branch 3 times, most recently from 5bf4f6e to 4bfffea Compare January 6, 2020 16:02
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.

2 participants