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

Added toIterator method and test cases. #275

Open
wants to merge 21 commits into
base: ee
Choose a base branch
from

Conversation

GoodLuckMister
Copy link

  • tests and linter show no problems (npm t)
  • tests are added/updated for bug fixes and new features
  • code is properly formatted (npm run fix)
  • description of changes is added in CHANGELOG.md
  • update .d.ts typings

Sorry, something went wrong.

Copy link
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

Add class EventIterator

lib/events.js Outdated
Comment on lines 83 to 88
return new Promise((resolve, reject) => {
if (error) reject(error);
const value = data.shift();
if (value) resolve({ value, done: false });
else promises.push({ resolve, reject });
});
Copy link
Member

Choose a reason for hiding this comment

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

  • if (error) return Promise.reject(error);
  • We need flag isPending: boolean

lib/events.js Outdated
Comment on lines 58 to 59
const data = [];
const promises = [];
Copy link
Member

Choose a reason for hiding this comment

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

We need no arrays, just single values

lib/events.js Outdated
},
return() {
handleError();
return Promise.resolve({ value: undefined, done: true });
Copy link
Member

Choose a reason for hiding this comment

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

  • Need variable or field this.done: boolean

@tshemsedinov
Copy link
Member

@GoodLuckMister still waiting for fixes

Copy link
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

test/events.js Outdated
Comment on lines 111 to 114
// eslint-disable-next-line no-unused-vars
for await (const event of iteratorWithError) {
looped = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// eslint-disable-next-line no-unused-vars
for await (const event of iteratorWithError) {
looped = true;
}
for await (const event of iteratorWithError) {
looped = true;
assert.ok(event);
}

test/events.js Outdated
Comment on lines 100 to 101
});
await testCase.test('error', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
});
await testCase.test('error', async () => {
});
await testCase.test('error', async () => {

lib/events.js Outdated
@@ -1,5 +1,68 @@
'use strict';

class EventIterator {
promise;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
promise;
promise = null;

lib/events.js Outdated
Comment on lines 5 to 7
isPending = false;
value = undefined;
done = false;
Copy link
Member

Choose a reason for hiding this comment

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

It is right place for iterator state done:boolean but not ok for isPending because it is related to one single step of iteration

Copy link
Member

Choose a reason for hiding this comment

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

promise is also related to a single step

Copy link
Member

Choose a reason for hiding this comment

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

value is also attribute of a iteration step, so we can hold this.step = { pending: boolean, data: { value, done } }, it also desirable to be private this.#step bot better just create such a structure and pass is over functional contexts.

lib/events.js Outdated
Comment on lines 61 to 63
[Symbol.asyncIterator]() {
return this;
}
Copy link
Member

Choose a reason for hiding this comment

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

Contract mixed: AsyncItarable, AsyncItarator, AsyncItaratorStep

test/events.js Outdated
[2],
['stop'],
];
for await (const event of iterator) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for await (const event of iterator) {
for await (const event of ee) {

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.

None yet

3 participants