[utils/streams] implement some generic stream helpers by spalger · Pull Request #10187 · elastic/kibana

spalger · 2017-02-03T22:16:30Z

This is the first supporting pr of several that was pulled out of #9853 in an attempt to make it slightly more digestible.

While working on #9853 I have needed to deal with streams in several different ways and along the way ended up with a core set of stream helpers. Since these helpers are not browser or server specific, and are used across components, and tested with generic use in mind I put them in src/utils.

This PR does not include use of the helpers beyond the tests

While working on pr 9853 I have needed to deal with streams in several different ways and along the way ended up with a core set of stream helpers. Since these helpers are not browser or server specific, and are used across components, and tested with generic use in mind I put them in src/utils.

kimjoar · 2017-02-07T18:47:07Z

src/utils/streams/__tests__/concat_stream.js

+      createConcatStream([])
+    ]);
+
+    expect(output).to.eql([1,2,3]);


It isn't really clear what the empty array input does on the concat. Maybe [0] or something, to make it clearer?

Maybe [0] or something, to make it clearer?

Not sure what you mean. You understand what it's doing though?

I meant createConcatStream([0]) or something like that. Won't that end up as [0,1,2,3]? Might make the param a bit "clearer" in what role it has.

Sure, sounds good

kimjoar · 2017-02-07T18:47:35Z

src/utils/streams/__tests__/concat_stream.js

+      expect(output).to.eql('abc');
+    });
+
+    it('works with strings', async () => {


arrays actually

kimjoar · 2017-02-07T18:53:11Z

src/utils/streams/__tests__/intersperse_stream.js

+    ).to.be('to be or not to be');
+  });
+
+  it('emits the intersperse value right before the second value, does not wait', async () => {


Why is this important?

I was trying to ensure that this isn't holding onto chunks it doesn't need to. I'll pick a better title

kimjoar · 2017-02-07T19:16:53Z

src/utils/streams/__tests__/split_stream.js

+
+describe('splitStream', () => {
+  it('splits buffers, produces strings', async () => {
+    const output = await split(createSplitStream('&'), [Buffer.from('foo&bar')]);


In other tests you use createPromiseFromStreams, should these also do that? (Or maybe good reason they're not?)

Started using the createPromiseFromStreams helper in the split function above

kimjoar · 2017-02-07T19:24:18Z

src/utils/streams/promise_from_streams.js

@@ -0,0 +1,69 @@
+import { race } from 'bluebird';


kimjoar · 2017-02-07T19:26:48Z

src/utils/streams/promise_from_streams.js

+ *  the promise to be rejected with that error.
+ *
+ *  @param  {Array<Stream>} streams
+ *  @return {Promise<undefined>}


The undefined doesn't match the docs above: "will be provided as the promise value"

…eam-utils

epixa · 2017-02-10T16:44:12Z

src/utils/streams/list_stream.js

+ *  @param  {Array<any>} items - the list of items to provide
+ *  @return {Readable}
+ */
+export function createListStream(items) {


Any reason you didn't use a default argument here rather than the items || [] check on the next line?

epixa · 2017-02-10T20:38:42Z

src/utils/streams/promise_from_streams.js

+ *  writing/reading.
+ *
+ *  If the last stream is readable, it's final value
+ *  will be provided as the promise value.


Can you explain the utility of this to me? I can't think of a practical scenario where the final value received in a stream should fulfill a promise.

It's used constantly in the tests with the concat stream, which always produces a single value. It's also the behavior of the Observable#toPromise() function in RxJS.

That said, I agree that outside of tests and systems that use streams exclusively, for everything, this feature has limited value.

…eam-utils

epixa · 2017-02-10T21:39:26Z

Since this is basically just some standalone utilities that aren't used yet, I can't really evaluate this in terms of whether this should even exist at all, but I trust you that this is necessary for the broader test framework changes. The code is in great shape, and the test coverage is there.

LGTM

Backports PR #10187 **Commit 1:** [utils/streams] implement some generic stream helpers While working on pr 9853 I have needed to deal with streams in several different ways and along the way ended up with a core set of stream helpers. Since these helpers are not browser or server specific, and are used across components, and tested with generic use in mind I put them in src/utils. * Original sha: a2f437f * Authored by spalger <spalger@users.noreply.github.com> on 2017-02-03T22:07:24Z **Commit 2:** Merge branch 'master' of github.com:elastic/kibana into implement/stream-utils * Original sha: b638d21 * Authored by spalger <spalger@users.noreply.github.com> on 2017-02-08T19:17:01Z **Commit 3:** [utils/streams] address kim's feedback * Original sha: 8566549 * Authored by spalger <spalger@users.noreply.github.com> on 2017-02-08T19:22:11Z **Commit 4:** Merge branch 'master' of github.com:elastic/kibana into implement/stream-utils * Original sha: a93df5a * Authored by spalger <spalger@users.noreply.github.com> on 2017-02-10T20:47:20Z **Commit 5:** [utils/stream/list] move default value into arg list * Original sha: 747218e * Authored by spalger <spalger@users.noreply.github.com> on 2017-02-10T20:54:42Z

spalger added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// review v5.3.0 v6.0.0 labels Feb 3, 2017

spalger requested a review from epixa February 3, 2017 22:16

spalger force-pushed the implement/stream-utils branch 6 times, most recently from 4bc229e to 1b32eb5 Compare February 3, 2017 23:34

spalger requested a review from kimjoar February 3, 2017 23:48

spalger mentioned this pull request Feb 3, 2017

WIP: [functional-tests] replace intern with custom test runner #9853

Closed

spalger force-pushed the implement/stream-utils branch from 1b32eb5 to a2f437f Compare February 4, 2017 04:42

kimjoar reviewed Feb 7, 2017

View reviewed changes

spalger added 2 commits February 8, 2017 12:17

Merge branch 'master' of github.com:elastic/kibana into implement/str…

b638d21

…eam-utils

[utils/streams] address kim's feedback

8566549

kimjoar approved these changes Feb 8, 2017

View reviewed changes

epixa removed the v5.3.0 label Feb 10, 2017

epixa reviewed Feb 10, 2017

View reviewed changes

spalger added 2 commits February 10, 2017 13:47

Merge branch 'master' of github.com:elastic/kibana into implement/str…

a93df5a

…eam-utils

[utils/stream/list] move default value into arg list

747218e

spalger merged commit 3a272e8 into elastic:master Feb 11, 2017

spalger added the v5.4.0 label Feb 11, 2017

elastic-jasper mentioned this pull request Feb 11, 2017

[5.x] [utils/streams] implement some generic stream helpers #10299

Merged

spalger mentioned this pull request Feb 15, 2017

[esArchiver] combine elasticDump and ScenarioManager #10359

Merged

spalger mentioned this pull request Mar 28, 2017

[functionalTestRunner] replace intern #10910

Merged

2 tasks

spalger deleted the implement/stream-utils branch October 18, 2019 17:41

Conversation

spalger commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimjoar Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epixa commented Feb 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

spalger commented Feb 3, 2017 •

edited

Loading

kimjoar Feb 8, 2017 •

edited

Loading

spalger Feb 8, 2017 •

edited

Loading