Upgrading to 1.4.1 broke Get* method generation for batch definitions

[d-natoli]

Unsure of what the cause is, as I haven't dug through the code, but we noticed when we upgraded the ts-proto lib from 1.4.0 to 1.4.1, the Get* methods were no longer automatically generated for batch service definitions. We rolled back to 1.4.0 and they were generated again.

[stephenh]

@d-natoli hm, can you try adding option 'context=true' to the gradle file?

I.e. like the "example_option1=true" here: https://github.com/google/protobuf-gradle-plugin

I have been trying/slowly to make ts-proto more general than just what we needed for twirp/remind, so I think made the context param (which is needed for the batch method) opt-in.

[hamiltop]

I think everything works now on 1.10.0. Would that make sense @stephenh ?

[stephenh]

@hamiltop yeah, you'll need to add context=true to the plugin/gradle config b/c that's not the default output anymore, but yeah otherwise afaiu it should be good.

https://github.com/stephenh/ts-proto/blob/master/src/utils.ts#L37

[hamiltop]

Got it. So the new dataloader logic is a little confusing. What's the purpose of getDataloaders? The examples I see in integrations is basically just a memoization function. Any way that can be made first class somehow? Maybe exporting a default function like:

export type DataLoaderProvider = <T>(id: string, cstr: () => T) => T

export getDefaultDataLoaderProvider() : DataLoaderProvider {
  const dataLoaders = new Map<string, any>();

  return function <T>(id: string, cstr: () => T): T {
    if (!dataLoaders.has(id)) {
      dataLoaders.set(id, cstr());
    }
    return dataLoaders.get(id);
  }
}

export defaultDataLoaderContext = {
  getDataLoader: getDefaultDataLoaderProvider();
}

// in other code
const ctx = {
  getDataLoader: getDefaultDataLoaderProvider();
}

// or simpler

const ctx = {...defaultDataLoaderContext, ...requestContext}

Thoughts?

[stephenh]

Ah, yeah, that makes sense. It took me awhile to remember why I made this change.

I think internally our approach to "how can a client do per-request batching" was to make a brand new client instance on every request.

This always felt a little odd to my old-school "services / clients are nearly always effective singletons", so this approach was going to allow creating the FooClients once to be reused across multiple concurrent requests, and then lean on the Context to provide the request-scoped DataLoader instances.

I like your idea of providing users an out-of-the-box impl of a DataLoaderProvider.

I.e. this makes sense:

// in other code
const ctx = {
  getDataLoader: newDataLoaderProvider();
}

This one might have a bug though where the dataloaders are captured/reused across contexts:

const ctx = {...defaultDataLoaderContext, ...requestContext}

It's been awhile since I made the getDataLoader change, so I forgot it'll be breaking for you guys, but shouldn't be too bad to move over? Definitely want to have you stay on latest / help drive future development / etc.

I should also add a readme entry about this auto-batching behavior b/c right now its just kind of a secret flag you have to already know about.

[stephenh]

Added a little note to the readme re auto-batching. Pretty light/could probably use its own section of the docs / blog post / etc.

[hamiltop]

Also, we should export the DataLoaderProvider type and the type for getDataLoader. Right now I have to copy/paste <T>(l: string, f: () => T) => T and it's confusing at best.

[stephenh]

we

...we as in I should look for your PR soon, or we as in I'll put that in when I get a chance? :-)

[hamiltop]

https://github.com/stephenh/ts-proto/pull/33

[stephenh]

Generally assuming you guys are fine here; happy to reopen if something is still blocking.