Use async as a suffix rather than a prefix for asynchronous variants.

[robertwb]

This is better aligned with standard javascript convention.

Often libraries have the default version be asynchronous, and
name the Sync one explicitly, we are marking the async ones as
they are by far the most common and usable for pipeline
construction.

---

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

• Choose reviewer(s) and mention them in a comment (R: @username).
• Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
• Update CHANGES.md with noteworthy changes.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

[robertwb]

R: @pskevin

[pskevin]

LGTM.

[pabloem]

I was wondering if we should default to making all apply calls be Async (they would return some sort of Future) and we could have futures applied on futures... wdyt Robert?

This is of course a big change to the API - I'm just spitballing...

[robertwb]

I did consider this, but it would make chaining much more verbose. E.g. instead of

lines
    .map((s: string) => s.toLowerCase())
    .flatMap(function* splitWords(line: string) {
      yield* line.split(/[^a-z]+/);
    })
    .apply(beam.countPerElement())

one would have to write

(await 
    (await 
        (await lines
            .map((s: string) => s.toLowerCase())
        ).flatMap(function* splitWords(line: string) {
      yield* line.split(/[^a-z]+/);
    })
    ).apply(beam.countPerElement()))

or

lines
   .map((s: string) => s.toLowerCase())
   .then((result) => {
      return result.flatMap(function* splitWords(line: string) {
        yield* line.split(/[^a-z]+/);
      })
    })
    .then((result) => {
      return result.apply(beam.countPerElement());
    })

which gets really painful. (Note that the future here is Javascript's Promise, which is a kind of special builtin).

[pabloem]

I thought of something like this:

chainedPromises = lines
  .map((s: string) => s.toLowerCase())
  .flatMap(function* splitWords(line: string) {
        yield* line.split(/[^a-z]+/);
      })
    .apply(beam.countPerElement());

countPcoll = chainedPromises.resolve();

// In case of error while expanding an intermediate PCollection, then we can throw/return an error from there when resolving the PCollection
// (error e.g. to resolve schema, or to match input/output types / etc).

Where each apply(...) returns a Promise<PCollection<...>> and the actual expansion is deferred...

though I haven't researched JS enough, I think this is doable...

[robertwb]

To do this one would have to add methods like flatMap to the Promise prototype (affecting all Promises across Javascript) or return a subclass (well, really a hierarchy of subclasses) of Promise (which I looked into and looks pretty sketchy). This would work if Promise could be a proxy object to the type it was promising, but it's not.

It looks like another alternative is

lines
    .invoke("map", (s: string) => s.toLowerCase())
    .invoke("flatMap", function* splitWords(line: string) {
      yield* line.split(/[^a-z]+/);
    })
    .invoke("apply", beam.countPerElement())

but this is still a bit icky, and looses type safety.

[pabloem]

Well thanks for doing the research and sharing your thoughts:)

…

On Mon, Jul 11, 2022, 12:45 PM Robert Bradshaw ***@***.***> wrote: To do this one would have to add methods like flatMap to the Promise prototype (affecting all Promises across Javascript) or return a subclass (well, really a hierarchy of subclasses) of Promise (which I looked into and looks pretty sketchy). This would work if Promise could be a proxy object to the type it was promising, but it's not. It looks like another alternative is lines .invoke("map", (s: string) => s.toLowerCase()) .invoke("flatMap", function* splitWords(line: string) { yield* line.split(/[^a-z]+/); }) .invoke("apply", beam.countPerElement()) but this is still a bit icky, and looses type safety. — Reply to this email directly, view it on GitHub <#22134 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ5Z3FKBOP5NE5BJBACZBLVTR2WLANCNFSM52N5F5TA> . You are receiving this because you commented.Message ID: ***@***.***>