FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: support passing generator functions into pipeline by ronag · Pull Request #31223 · nodejs/node · GitHub

/ node Public

stream: support passing generator functions into pipeline - #31223

Closed
ronag wants to merge 24 commits into
nodejs:masterfrom
nxtedition:pipeline-iterable
Closed

stream: support passing generator functions into pipeline#31223
ronag wants to merge 24 commits into
nodejs:masterfrom
nxtedition:pipeline-iterable

Conversation

ronag commented Jan 6, 2020
edited
Loading

Copy link
Copy Markdown
Member

Add support for generators and functions in pipeline.

This makes it possible to do the following:

await pipeline(
  async function* () {
    yield await read();
  },
  async function*(source) {
    await for (const chunk of source) {
      yield chunk;
    }
  },
  async function(source) {
    await for (const chunk of source) {
      await write(chunk):
    }
  }
);
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 6, 2020
ronag force-pushed the pipeline-iterable branch 10 times, most recently from 92c9e6f to db3aaa3 Compare January 6, 2020 16:21
BridgeAR requested review from lpinca and mcollina January 6, 2020 19:01

mcollina commented Jan 6, 2020

Copy link
Copy Markdown
Member

I would prefer we did not pass through the stream equivalents for pipeline - I'm not sure if it's feasible. The main reason is performance: streams adds a lot of overhead and a pipeline composed by async iterators can be extremely more performant.

I'm not sure if this is feasible or just a dream.

ronag commented Jan 6, 2020

Copy link
Copy Markdown
Member Author

I'm not sure if this is feasible or just a dream.

Might be feasible. I've updated the PR.

ronag force-pushed the pipeline-iterable branch 13 times, most recently from d7035ec to 227642e Compare January 6, 2020 21:25

ronag commented Jan 6, 2020
edited
Loading

Copy link
Copy Markdown
Member Author

@mcollina: I think you will like this iteration. Now it's possible to do e.g.

let res = '';
pipeline(async function*() {
  await new Promise((resolve) => process.nextTick(resolve));
  yield 'hello';
  yield 'world';
}, async function*(source) {
  for await (const chunk of source) {
    yield chunk.toUpperCase();
  }
}, async function(source) {
  for await (const chunk of source) {
    res += chunk;
  }
}, common.mustCall((err) => {
  assert.strictEqual(err, undefined);
  assert.strictEqual(res, 'HELLOWORLD');
}));

Without passing through the stream equivalents for pipeline.

Still needs some work to ensure edge cases are covered and errors are properly thrown. WIP label please.

ronag force-pushed the pipeline-iterable branch from 227642e to d26e808 Compare January 6, 2020 21:28
Comment thread doc/api/stream.md

nodejs-github-bot commented Jan 11, 2020
edited by Trott
Loading

Copy link
Copy Markdown
Collaborator

Trott commented Jan 11, 2020

Copy link
Copy Markdown
Member

CITGM looks good.

ronag mentioned this pull request Jan 12, 2020
4 tasks
Comment thread doc/api/stream.md

Copy link
Copy Markdown
Collaborator

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I’m lost on why this is semver-major. Can you recap why on the PR description?

ronag commented Jan 15, 2020
edited
Loading

Copy link
Copy Markdown
Member Author

I’m lost on why this is semver-major. Can you recap why on the PR description?

I think it was in an earlier version but I can't see either that it would be now.

ronag commented Jan 15, 2020

Copy link
Copy Markdown
Member Author

@Trott: I think we can remove semver-major as @mcollina pointed out.

Trott commented Jan 18, 2020

Copy link
Copy Markdown
Member

Landed in 7b78ff0

ronag commented Jan 18, 2020

Copy link
Copy Markdown
Member Author

Notable change?

Copy link
Copy Markdown
Member

@ronag i tried to backport this to v13.x, but it seems to have broken CI; See https://travis-ci.com/nodejs/node/jobs/288147410

ronag commented Feb 17, 2020

Copy link
Copy Markdown
Member Author

@codebytere: Yes, it seems eos is calling the callback earlier in 13.x. I'll make a fix for master which we can backport.

Copy link
Copy Markdown
Contributor

@ronag does this still need a backport?

ronag commented Mar 9, 2020

Copy link
Copy Markdown
Member Author

@MylesBorins I've already backported it #31975. Did I miss a label or something?

Copy link
Copy Markdown
Contributor

@ronag thanks! when something has been backported we usually apply a different label. backport-open once it has been opened and backported-to when it has landed

targos commented Apr 25, 2020

Copy link
Copy Markdown
Member

Depends at least on #30869 to land on v12.x

ronag commented Apr 25, 2020

Copy link
Copy Markdown
Member Author

@targos: I don't think this should land on v12

targos commented Apr 25, 2020

Copy link
Copy Markdown
Member

@ronag why not? I we do not backport the recent stream changes, this subsystem will be really difficult to maintain on v12. There are almost conflicts with every pull request.

ronag commented Apr 25, 2020

Copy link
Copy Markdown
Member Author

@ronag why not? I we do not backport the recent stream changes, this subsystem will be really difficult to maintain on v12. There are almost conflicts with every pull request.

Disregard my previous comment. You are right.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.


Back | FazBrowse Home | New Git URL