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

Add ability to use Observables with async/await #83

Merged
merged 1 commit into from Oct 28, 2022

Conversation

nyteshade
Copy link
Contributor

@nyteshade nyteshade commented Oct 27, 2022

A lot of modern JavaScript code uses async/await to handle asynchronous code. This is a great way to write code that is easy to read and understand. However, it is not possible to use async/await with Observables outside of the forEach() which requries more boilerplate than needed in order to do so.

Changes:

  • Adds .all() to an observable instance allowing one to await the values it contains
  • Adds a test for the new functionality

@nyteshade
Copy link
Contributor Author

@zenparsing Please take a look when you can. I'm hoping to get this change in so it can be used upstream by Apollo GraphQL.

@nyteshade
Copy link
Contributor Author

@zenparsing fixed merge conflict

@zenparsing
Copy link
Owner

Adding support for for await...of would be cool - the issue has always been that it implies unbounded buffering in the case where the observable produces values faster than the async iterator (or body of the for await...of loop) pulls them out. How you want to deal with that depends on the use case - sometimes you want to buffer everything (as your PR effectively does), and sometimes you don't (for example, when iterating over a high-frequency event stream, like mouse moves).

But it would be cool - I'll give it some thought.

@zenparsing
Copy link
Owner

zenparsing commented Oct 28, 2022

Currently, you have to do something like this, right?

let list = [];
await observable.forEach(value => list.push(value));
for (const value of list) {
  // loop body...
}

or

await observable.forEach(value => {
  // loop body...
});

@nyteshade
Copy link
Contributor Author

nyteshade commented Oct 28, 2022

Similar but yes. That all data will be retrieved beforehand is called out in the notes. It is set up as an asyncIterator so it’s used with

for await (let item of observable)

If the user wants to traverse the values as they arise the traditional methods still apply. Same as with the promisify() method. It gets all values in order and returns them in a promise fulfilled array. Again, traditional methods still apply.

These add only an opportunistic functional add for those who know the data coming. If the wait is a network request or two, one would have to wait regardless. So getting that data with a single line of code is nice.

@zenparsing
Copy link
Owner

If we wanted to support for await...of, we'd need to deliver values as they come in, rather than waiting until they are all available (similar to https://github.com/benlesh/rxjs-for-await). But that's where you get into the buffering problems.

So I understand correctly, can you give an example where:

await observable.forEach(value => {
  // loop body...
});

does not work?

@nyteshade
Copy link
Contributor Author

Wow, I will admit that I did not expect forEach to return a promise. It's method parameters and name does not imply a promise that resolved all its data would be the return value. I guess the thing is, for something like promisify() or for await (let item of observable), you do not need an inner function.

let values = await observable.forEach( still requires a function to be supplied. It requires more boilerplate than one would except but I do admit that it would work similarly otherwise. I apologize for not noticing that forEach returned a promise. Typically a function of that name has no return value.

@nyteshade
Copy link
Contributor Author

I'll address the package-lock issues once I know whether or not the PR has legs. @zenparsing

@zenparsing
Copy link
Owner

@nyteshade It's definitely not an obvious part of the API, although it's old and dates back to the original JS proposal 😄 It still might be nice to add something like the promisify method (although I'm thinking maybe naming it all):

for (const value of await observable.all(()) {
  // Loop body...
}

@nyteshade
Copy link
Contributor Author

All is fine and I can make that change. What would you like to do about the iterator? Keep it or kill it?

@nyteshade
Copy link
Contributor Author

@zenparsing test and function updated with the name all() instead of promisify()

@@ -315,6 +315,26 @@ export class Observable {
}));
}

async all() {
Copy link
Owner

Choose a reason for hiding this comment

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

Can all just be this?

async all() {
  let values = [];
  await this.forEach(value => values.push(value));
  return values;
}

return promise;
}

async *[Symbol.asyncIterator]() {
Copy link
Owner

Choose a reason for hiding this comment

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

With all, can we just do this?

for (const value of await observable.all()) {
  // Loop body...
}

I'd like to defer adding Symbol.asyncIterator because of the buffering issues.

test/all.js Outdated
let observable = Observable.of(1,2,3,4);
let values = await observable.all();

assert.deepEqual([1,2,3,4], values);
Copy link
Owner

Choose a reason for hiding this comment

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

The actual value should come first, and the expected value second, so:

assert.deepEqual(values, [1,2,3,4]);

.babelrc Outdated
@@ -0,0 +1,25 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

Let's leave out these changes to babel etc. in this PR. I'm going to upgrade the package to ESM and get rid of all this babel junk.

@zenparsing
Copy link
Owner

Sorry for all of the conflicts. If this PR just adds the all method (and the corresponding test), I'll merge.

README.md Outdated
@@ -174,3 +174,31 @@ Observable.of(1, 2, 3).concat(
```

Merges the current observable with additional observables.

### observable.promisify()
Copy link
Owner

Choose a reason for hiding this comment

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

You can leave out this change - I'll update the README after merge.

A lot of modern JavaScript code uses async/await to handle asynchronous code. This is a great way to write code that is easy to read and understand. However, it is not possible to use async/await with Observables outside of the `forEach()` which requries more boilerplate than needed in order to do so.

Changes:
 * Adds `.all()` to an observable instance allowing one to `await` the values it contains
 * Adds a test for the new functionality
@zenparsing
Copy link
Owner

Thanks - will push a new version soon.

@nyteshade
Copy link
Contributor Author

As you wish, though I spent more time than I'd like to admit fixing the babel stuff, lol. Et voila, you have it,

  • Only adding .add(), using forEach to procure values
  • No changes to README
  • Reverted babel updates
  • Removing [Symbol.asyncIterator]
  • Remove iterator test
  • Reversing position of arguments in assert.deepEqual()

It should be as you like now.

@nyteshade
Copy link
Contributor Author

@zenparsing btw, when I ran npm run build before committing, it generated a lib/Observable.js that used classes. There is an downstream library used by @apollo/client called zen-observable-ts that has tests that rely on your build process exporting pure functions. I don't know where you stand on this. class has been available in JS for nearly 10 years now so probably that is the desired path, but as I try to create a PR in zen-observable-ts, I've noticed the issues there. It's kinda their problem (and mine in a way) but let me know where you land and I'll try to perhaps adjust the their repo's tests.

@zenparsing
Copy link
Owner

I was wondering about that, so I bumped the version to 0.9.0. I'd really like to stop transpiling anything now (other than modules), but let me know if it's a blocker.

@nyteshade
Copy link
Contributor Author

I guess technically in SemVer 0.9.0 is a backwards compatible feature release. To indicate breaking changes, a bump from 0.8.15 to 1.0.0 would be required. If there is a way to generate non-class based output you could do a 0.9.1 release. Then save the class based output until you remove transpilers and do a 1.0 release @zenparsing

@nyteshade
Copy link
Contributor Author

I can try to coax Babel into doing so unless you have an older version of node. I’m on a M1 Mac so I need to use Rosetta to get older node versions running.

@nyteshade
Copy link
Contributor Author

nyteshade commented Oct 28, 2022

Oh actually, @zenparsing, the behavior changed in this commit d3d16f2. It uses es2018 for output which will use classes because the whole world today supports them. I would recommend undoing that until you move to a 1.0.0 release. As it will break dependencies that incorrectly rely on Observable being a function.

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

2 participants