support Iterable objects, such as Map, Set, String #415

KHYehor · 2019-03-17T19:53:49Z

I added support for iterable objects using [Symbol.iterator],
also tests are written

lib/array.js

o-rumiantsev · 2019-03-18T08:16:59Z

It is better to use common.iter to iterate over the iterable.

mille-nium · 2019-03-18T08:34:12Z

lib/array.js

-      if (done) result[count] = itemResult;
+      const itemResult = fn(data.next().value, count);
+      if (done) {
+        if (name === 'Array') result.push(itemResult);


FWIW, an object with these types as keys and an appropriate mapping functions as values looks prettier for purpose like this. I suppose, that this piece of code will not be relevant after using common.iter, however just pointing it out for future reference.

May I ask you a question?
If I replace my const data = items[Symbol.iterator]();
with const data = common.iter(items); I will get almost the same, and I have no need to rewrite code where I use data.next().value.
So, Is it what I am expected to do?

Yes, I've missconcepted this, you may need here to use a constructor name to add element to result. So it still make sense, AFAIS, to move this conditional statement to a separate function or to make an mapping object described previously, because it is used at least four times in this file, kind of boilerplate code, if I'm not mistaken.

@mille-nium What do you think about this solution with separate function?

const push = (name, result, value) => { switch (name) { case 'Set': return result.add(value); case 'Map': return result.set(value[0], value[1]); case 'Array': return result.push(value); case 'String': return result += value; } return null; }

I prefer not using switch-case here.

May I ask you again?
If I separate push function prettier code says that I should change my let result to const result How can I solute this? Can I turn off this option, or what?

Also, If I give string into object pushValue, I won't concat my string with string += value
If I do something like this, It will work

return; } if (name === 'String') result += value; else push(name, result, value); count++; if (count === len) done(null, result);

I wonder, if you could simply use common.Iterator.map() and others methods and then just common.Iterator.collectTo()?

May be, I will check.

I suppose, this was actually meant by @o-rumiantsev

lib/array.js

SemenchenkoVitaliy · 2019-03-18T09:22:33Z

test/array.every.js

@@ -60,7 +60,7 @@ metatests.test('every with two-element arrays', test =>
  )
 );

-metatests.test('every', test => {
+metatests.test('every / array', test => {
  const data = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16];


I think it can be moved to a separate variable.

What exactly should I move to separate variable?
const data = [...]

Yes, const data = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]. I don't mind different name though.

Still relevant.

test/array.find.js

o-rumiantsev · 2019-03-19T07:28:41Z

It is better to use common.iter to iterate over the iterable.

You can also use metasync.asyncIter instead of common.iter where needed

KHYehor · 2019-03-20T15:56:33Z

@o-rumiantsev I have one misunderstanding with you. It is said that
Asynchronous map (iterate parallel) and there isn't used any async/await or promises, only call back next(). If I insert common.iter I will get the same that I had. If I insert metasync.iter it will be some different, and I also get strange error messages Message: Failure after end: callback is not a function. Or I just can't understand something.

KHYehor · 2019-03-24T19:49:31Z

Now I have one inaccuracy. If I call method .fetch() then call other methods like .filter(), .reduce(), ... I will get wrong behavior. Because string this.execute() is async, and I return this which isn't calculated yet. It could be solved with Promise or async/await but you might solute something else.
test/chain.js 'for chain after fetch' I am talking about this one.

ArrayChain.prototype.fetch = function(fn) {
  this.chain.push({ op: 'fetch', fn });
  this.execute();
  return this;
};

KHYehor · 2019-03-25T14:07:41Z

@SemenchenkoVitaliy @belochub I noticed one interesting detail, I can be wrong so correct me if I am.
If I rewrite metatest 'for chain after fetch' to process.nextTick like this

metatests.test('for chain after fetch', test => {
  metasync
    .for([1, 2, 3, 4])
    .map((item, cb) => process.nextTick(() => cb(null, item * item)))
    .filter((item, cb) => process.nextTick(() => cb(null, item > 5)))
    .fetch((error, result, resume) => {
      test.error(error);
      test.strictSame(result, [9, 16]);
      process.nextTick(() => resume(null, result));
    })
    .filter((item, cb) => {
      process.nextTick(() => cb(null, item > 10));
    })
    .map((item, cb) => {
      process.nextTick(() => cb(null, --item));
    })
    .fetch((error, result) => {
      test.error(error);
      test.strictSame(result, [15]);
      test.end();
    });
});

I will get async functions, but it doesn't work as should, I will get errors. (it is master)
And If I rewrite all methods to asyncIter as I made, I will get the same errors with any fn.
How should it be solute?

KHYehor · 2019-03-29T13:02:08Z

In last commit I have described my problem with lib/chain.js I am trying to find the solution for this. And you can check and say what you think about lib/array.js.
P.S. Tests failed because of reserved words await and next() in v6.x and v8.x

lib/array.js

lib/chain.js

SemenchenkoVitaliy · 2019-04-03T10:59:09Z

test/array.every.js

@@ -60,7 +60,7 @@ metatests.test('every with two-element arrays', test =>
  )
 );

-metatests.test('every', test => {
+metatests.test('every / array', test => {
  const data = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16];


Still relevant.

lib/array.js

nechaido · 2019-04-05T12:20:53Z

lib/array.js

+  const promise =
+    isArray || items.constructor.name === 'String'
+      ? data.toArray()
+      : data.collectTo(items.constructor);


After consulting with @belochub, we agreed that we should return an Iterator object if items is not an Array.

lib/array.js

lib/control.js

lib/array.js

nechaido

Majority of comments apply to every function.

lib/array.js

nechaido · 2019-04-19T09:15:06Z

lib/array.js

@@ -231,16 +191,17 @@ const REDUCE_RIGHT_EMPTY_ARR =
 //       argument in first iteration
 const reduceRight = (items, fn, done, initial) => {


reduceRight should probably be implemented on AsyncIterator.

lib/array.js

lib/control.js

lib/array.js

nechaido · 2019-04-23T09:54:42Z

lib/array.js

@@ -231,16 +191,17 @@ const REDUCE_RIGHT_EMPTY_ARR =
 //       argument in first iteration
 const reduceRight = (items, fn, done, initial) => {


test/array.find.js

SemenchenkoVitaliy · 2019-04-23T11:16:52Z

test/array.each.js

+  );
+});
+
+// metatests.test('each with error', test => {


Why this is commented out?

Because now each works parallel and this callback callback(eachError); will not cause the stopping of processing other items.
What should I do with this test? rewrite for checking if other elements were processed?

SemenchenkoVitaliy · 2019-04-23T11:22:17Z

test/chain.js

      test.end();
    });
+  // .filter((item, cb) => process.nextTick(() => cb(null, item < 4)))


This problem with fetch after fetch wasn't solved by me. Ask @nechaido.

nechaido · 2019-05-06T08:44:16Z

lib/array.js

+    .parallel(item => promisify(fn)(item))
+    .then(res => {
+      const accepted = res.every(predicateResult => predicateResult);
+      done(null, accepted);


Suggested change

done(null, accepted);

done(null, res.every(e => e);

nechaido · 2019-05-06T08:57:33Z

lib/async-iterator.js

@@ -213,6 +221,16 @@ class MapIterator extends AsyncIterator {
  }
 }

+class ReverseIterator extends AsyncIterator {
+  constructor(base) {
+    const newBase = [];


This should be done on the first next invocation. base is always AsyncInterotor.

lib/async-iterator.js

nechaido · 2019-05-24T11:16:38Z

lib/array.js

-  }
+  const isArray = Array.isArray(items);
+  asyncIter(items)
+    .parallel(item => promisify(fn)(item))


Suggested change

.parallel(item => promisify(fn)(item))

.parallel(promisify(fn))

nechaido · 2019-05-24T11:18:21Z

lib/array.js

-  next();
+  const isArray = Array.isArray(items);
+  const iter = asyncIter(items)
+    .map(item => promisify(fn)(item))


Suggested change

.map(item => promisify(fn)(item))

.map(promisify(fn))

nechaido · 2019-05-27T11:24:23Z

test/array.find.js

 });

-metatests.test('with array without element which is searching', test => {
-  const data = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16];
+metatests.test('find without element which is searching', test => {


Suggested change

metatests.test('find without element which is searching', test => {

metatests.test('find without the desired element', test => {

nechaido · 2019-05-27T11:33:52Z

test/array.reduce.js

 });

-metatests.test('reduce without initial and with single-element array', test => {
+metatests.test('reduce single-element array without initial', test => {
  const arr = [2];

  metasync.reduce(
    arr,
    (prev, cur, callback) => process.nextTick(() => callback(null, prev + cur)),


Suggested change

(prev, cur, callback) => process.nextTick(() => callback(null, prev + cur)),

test.mustNotCall(),

ivan-tymoshenko · 2019-05-27T11:26:27Z

lib/array.js

-    done(null, []);
+//     result - <Iterable>
+const map = (items, fn, done = common.emptiness) => {
+  if (!items[Symbol.iterator]) {


If I understand correctly, Iterable can also contain Symbol.asyncIterator and do not contain Symbol.iterator.

const asyncIterable = { [Symbol.asyncIterator]() { return { i: 0, next() { if (this.i < 3) { return Promise.resolve({ value: this.i++, done: false }); } return Promise.resolve({ done: true }); } }; } }; (async function() { for await (const i of asyncIterable) { console.log(i); } })();

AFAIU we can omit this check because AsyncIterator constructor will make them.

ivan-tymoshenko · 2019-05-27T11:33:13Z

lib/array.js

-
-  if (!items.length) {
-    if (done) done(null, []);
+  if (!items[Symbol.iterator]) {


ivan-tymoshenko · 2019-05-27T11:40:10Z

lib/array.js

-  if (!len) {
-    done(null, []);
+const filter = (items, fn, done = common.emptiness) => {
+  if (!items[Symbol.iterator]) {


lib/array.js

nechaido · 2019-06-26T13:57:29Z

ping @belochub.

nechaido

Please, split this PR into two commits: one that adds support for Iterables and the one that "updates" tests.

nechaido · 2019-07-01T10:27:08Z

lib/array.js

+    .parallel(async item => [await promisify(fn)(item), item])
+    .then(res => {
+      const filtered = res
+        .filter(([predicateResult]) => predicateResult)


common.Iterator.filterMap ?

nechaido · 2019-07-01T11:00:13Z

test/array.asyncMap.js

  const arr = [1, 2, 3];
  const expectedArr = [2, 4, 6];

  metasync.asyncMap(
    arr,
-    item => item * 2,


I'd prefer to leave this test the way it is, to ensure that synchronous functions are still supported.

nechaido · 2019-07-01T11:14:12Z

test/array.every.js

@@ -26,23 +26,23 @@ const fewStrictSameResult = (inOutPairs, test) => {

 metatests.test('every with error', test => {
  const data = [1, 2, 3];
-  const everyErr = new Error('Every error');
+  const everyError = new Error('Every error');


I'd prefer not to rename variables.

nechaido · 2019-07-01T11:16:15Z

test/array.every.js

-metatests.test('every with empty array', test =>
-  strictSameResult([], true, test, () => test.end())
-);
+metatests.test('every with empty', test => {


do we really need to change this?

PR-URL: #415

belochub · 2019-07-03T12:15:37Z

lib/array.js

+//     result - <Iterable>
+const map = (items, fn, done = common.emptiness) => {
+  const isArray = Array.isArray(items);
+  asyncIter(items)


I think wrapping everything in AsyncIterator and additionally using util.promisify() adds unnecessary overhead, that can be avoided by just rewriting the original functions with iterators in mind. This also won't require any changes to metasync.AsyncIterator. If we decide to keep it the way it is in this PR we should probably at least benchmark this variant to make sure this doesn't lead to significant performance regressions.
/cc @tshemsedinov

KHYehor force-pushed the support-iterable branch from ce1bb46 to 4596e10 Compare March 17, 2019 20:04

KHYehor requested a review from belochub March 17, 2019 20:07

nechaido requested a review from o-rumiantsev March 18, 2019 07:59

belochub requested review from nechaido and mille-nium and removed request for o-rumiantsev March 18, 2019 07:59

nechaido requested a review from o-rumiantsev March 18, 2019 08:01

SemenchenkoVitaliy reviewed Mar 18, 2019

View reviewed changes

lib/array.js Outdated Show resolved Hide resolved

lib/array.js Outdated Show resolved Hide resolved

lib/array.js Outdated Show resolved Hide resolved

lib/array.js Outdated Show resolved Hide resolved

mille-nium reviewed Mar 18, 2019

View reviewed changes

SemenchenkoVitaliy reviewed Mar 18, 2019

View reviewed changes

SemenchenkoVitaliy reviewed Apr 3, 2019

View reviewed changes

nechaido reviewed Apr 3, 2019

View reviewed changes

lib/array.js Outdated Show resolved Hide resolved

nechaido reviewed Apr 5, 2019

View reviewed changes

lib/array.js Outdated Show resolved Hide resolved

KHYehor force-pushed the support-iterable branch from eabbd1d to 1070a5f Compare April 5, 2019 14:11

nechaido requested changes Apr 19, 2019

View reviewed changes

KHYehor force-pushed the support-iterable branch from f477971 to dc722a5 Compare April 19, 2019 19:41

nechaido reviewed Apr 23, 2019

View reviewed changes

SemenchenkoVitaliy reviewed Apr 23, 2019

View reviewed changes

nechaido assigned o-rumiantsev May 2, 2019

o-rumiantsev force-pushed the support-iterable branch from 3c1d416 to a65fd11 Compare May 3, 2019 16:47

nechaido reviewed May 6, 2019

View reviewed changes

lib/async-iterator.js Show resolved Hide resolved

nechaido reviewed May 14, 2019

View reviewed changes

lib/async-iterator.js Show resolved Hide resolved

o-rumiantsev requested a review from nechaido May 24, 2019 05:36

nechaido reviewed May 24, 2019

View reviewed changes

nechaido reviewed May 27, 2019

View reviewed changes

ivan-tymoshenko reviewed May 27, 2019

View reviewed changes

nechaido requested a review from ivan-tymoshenko May 27, 2019 12:55

nechaido approved these changes May 27, 2019

View reviewed changes

nechaido removed the request for review from o-rumiantsev May 27, 2019 12:56

nechaido closed this Jun 10, 2019

nechaido reopened this Jun 18, 2019

o-rumiantsev force-pushed the support-iterable branch from 42ebb93 to 304b2b7 Compare June 20, 2019 09:29

o-rumiantsev requested a review from nechaido June 20, 2019 09:33

ivan-tymoshenko approved these changes Jun 25, 2019

View reviewed changes

o-rumiantsev approved these changes Jun 27, 2019

View reviewed changes

nechaido reviewed Jul 1, 2019

View reviewed changes

o-rumiantsev force-pushed the support-iterable branch 2 times, most recently from ee7985b to 4f027ba Compare July 1, 2019 15:56

o-rumiantsev added a commit that referenced this pull request Jul 1, 2019

Add support for iterable objects

68ea25d

PR-URL: #415

o-rumiantsev added a commit that referenced this pull request Jul 1, 2019

Add tests for iterable support

4f027ba

PR-URL: #415

o-rumiantsev force-pushed the support-iterable branch from 4f027ba to 8cf8ca8 Compare July 1, 2019 16:04

o-rumiantsev added a commit that referenced this pull request Jul 1, 2019

Add support for iterable objects

2e9d482

PR-URL: #415

o-rumiantsev added a commit that referenced this pull request Jul 1, 2019

Add tests for iterable support

8cf8ca8

PR-URL: #415

o-rumiantsev added 2 commits July 1, 2019 19:07

Add support for iterable objects

6019af6

PR-URL: #415

Add tests for iterable support

d713492

PR-URL: #415

o-rumiantsev force-pushed the support-iterable branch from 8cf8ca8 to d713492 Compare July 1, 2019 16:08

belochub requested a review from tshemsedinov July 3, 2019 12:06

belochub requested changes Jul 3, 2019

View reviewed changes

		@@ -231,16 +191,17 @@ const REDUCE_RIGHT_EMPTY_ARR =
		// argument in first iteration
		const reduceRight = (items, fn, done, initial) => {

	.parallel(item => promisify(fn)(item))
	.parallel(promisify(fn))

	metatests.test('find without element which is searching', test => {
	metatests.test('find without the desired element', test => {

	(prev, cur, callback) => process.nextTick(() => callback(null, prev + cur)),
	test.mustNotCall(),

support Iterable objects, such as Map, Set, String #415

Are you sure you want to change the base?

support Iterable objects, such as Map, Set, String #415

Conversation

KHYehor commented Mar 17, 2019

o-rumiantsev commented Mar 18, 2019

mille-nium Mar 18, 2019 • edited

Choose a reason for hiding this comment

KHYehor Mar 18, 2019 • edited

Choose a reason for hiding this comment

mille-nium Mar 18, 2019 • edited

Choose a reason for hiding this comment

KHYehor Mar 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KHYehor Mar 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o-rumiantsev commented Mar 19, 2019 • edited

KHYehor commented Mar 20, 2019

KHYehor commented Mar 24, 2019

KHYehor commented Mar 25, 2019

KHYehor commented Mar 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KHYehor Apr 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido commented Jun 26, 2019

nechaido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mille-nium Mar 18, 2019 •

edited

KHYehor Mar 18, 2019 •

edited

mille-nium Mar 18, 2019 •

edited

KHYehor Mar 18, 2019 •

edited

KHYehor Mar 18, 2019 •

edited

o-rumiantsev commented Mar 19, 2019 •

edited

KHYehor commented Mar 29, 2019 •

edited

KHYehor Apr 24, 2019 •

edited