Node-slack-sdk: paginate should return an iterable

[BeLi4L]

Description

I can't use WebClient.paginate(method: string) in TypeScript.

What type of issue is this? (place an x in one of the [ ])

• [x] bug
• [ ] enhancement (feature request)
• [ ] question
• [ ] documentation related
• [ ] testing related
• [ ] discussion

Requirements (place an x in each of the [ ])

• [x] I've read and understood the Contributing guidelines and have done my best effort to follow them.
• [x] I've read and agree to the Code of Conduct.
• [x] I've searched for any related issues and avoided creating a duplicate issue.

---

Bug Report

Packages:

Select all that apply:

• [x] @slack/web-api
• [ ] @slack/events-api
• [ ] @slack/interactive-messages
• [ ] @slack/rtm-api
• [ ] @slack/webhooks
• [ ] I don't know

Reproducible in:

package version: 5.0.1

node version: 10.15.3

OS version(s): Ubuntu 18.04

Steps to reproduce:

Try to compile the pagination example code, or simply:

async function listUsers() {
  let users = [];
  for await (const page of web.paginate('users.list')) {
    users = [...users, ...page.members];
  }
  return users;
}

Expected result:

No compilation errors.

Actual result:

TypeScript throws the following error:

Type 'AsyncIterator<WebAPICallResult>' must have a '[Symbol.asyncIterator]()' method that returns an async iterator.

Possible solution

I guess that for await expects an iterable, not an iterator, so I fixed it by casting it as AsyncIterableIterator<...>, which is the actual return type: https://github.com/slackapi/node-slack-sdk/blob/master/packages/web-api/src/WebClient.ts#L217

[aoberoi]

@BeLi4L thanks for filing this! you're right, we made a mistake with that type. I actually think we should return a AsyncIterable<WebAPICallResult> because using AsyncIterableIterator<> is sort of revealing an implementation detail that isn't necessary for the for await...of loop to work.

Sounds like a pretty straight forward fix, but also it would be nice to verify this in the integration tests for types (located in https://github.com/slackapi/node-slack-sdk/blob/master/integration-tests/types/webclient-paginate-types.ts). It was an oversight to only test the functional (reducer) interfaces in that file.

Would you be willing to contribute a fix?

[clavin]

Changing the return type from an AsyncIterator to an AsyncIterable would be a breaking change, though. Would it make sense to change this to an AsyncIterator & AsyncIterable (which the implementation already supports because this is basically the definition of an AsyncIterableIterator) so the for-await syntax works now, then switch to just AsyncIterable on the next major version?

I'm happy to implement this change & update/add tests either way, just unsure of the direction to take.

[michaeldever]

Seeing this issue as well in Typescript.

[aoberoi]

@clavin my view is that if we always returned an object whose intended observable behavior was as an AsyncIterable, then changing the type AsyncIterator to AsyncIterable is not a breaking change but rather a correction of a bug.

This is def one of those gray areas when it comes to semver. I've got two reasons for for this view:

1. Intentions cannot be separated from semver. If they were, then any bugfix which alters the observable behavior is a backwards incompatible change because the new behavior isn't identical to the previous. Therefore semver should consider the intended API contract over the actual at a moment (there are always exceptions for extreme cases).
2. We don't semver on the TypeScript interface (but rather the JavaScript interface). Thee JS interface would not break here.

i'm open to a discussion if you disagree. also happy to accept a patch!

[clavin]

... then any bugfix which alters the observable behavior is a backwards incompatible change ...

Oh. Yeah. I think you hit the nail on the head here. Changing to the correct (inteded) type is definitely the preferred path here and I wouldn't see it as a breaking change.

[clavin]

🤔 Did some thinking and I realized that AsyncIterable means that Symbol.asyncIterator is defined and support for Symbol.asyncIterator was added in node v10... which is above our current minimum supported node version, v8. 🤕

💡 I don't think this blocks the bugfix (since we already polyfill the Symbol in the file) but I think it's an important mention in this issue.