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

Proposal: alternative interface that's more aligned with current idioms #23

Open
dpup opened this issue May 16, 2024 · 4 comments
Open
Labels
enhancement New feature or request

Comments

@dpup
Copy link
Owner

dpup commented May 16, 2024

Background

Currently the generated functions are exposed as static methods on a class. For example:

export class AccountService {
  static CreateAccount(
    this: void,
    req: CreateAccountRequest,
    initReq?: fm.InitReq,
  ): Promise<CreateAccountResponse> {
    return fm.fetchRequest<CreateAccountResponse>(`/api/accounts`, {
      ...initReq,
      method: 'POST',
      body: JSON.stringify(req, fm.replacer),
    });
  }
}

I don't know the original intent of this pattern, but you lose any benefits of having a Service instance that tracks state and you get typescript that's not very idiomatic.

import { AccountService } from './accounts.pb';

const resp = AccountService.CreateAccount(newAccount, {
  pathPrefix: API_HOST,
  credentials: 'include',
  headers: {
    'X-CSRF-Protection': 1
  }
})

Proposal

Introduce a flag use_static_classes which defaults to true for now. When use_static_classes is false, you simply get an exported function that maps to the method name:

export const createAccount = (
  req: CreateAccountRequest,
  initReq?: InitReq,
): Promise<CreateAccountResponse> => {
  {
    return fm.fetchRequest<CreateAccountResponse>(`/api/accounts`, {
      ...initReq,
      method: 'POST',
      body: JSON.stringify(req, fm.replacer),
    });
  }
};

And then:

import { createAccount } from './accounts.pb';

const resp = createAccount(newAccount, {
  pathPrefix: API_HOST,
  credentials: 'include',
  headers: {
    'X-CSRF-Protection': 1
  }
})

Problem : conflicting function names

The main downside I see in this approach is that there could be ambiguous function names in the case where a single proto file contains multiple services with the same method. This seems unlikely in my experience, but a possible fix could be to generate multiple TS files.

For example: consider accounts.proto contains AccountService and AdminService, both with createAccount methods.

We could generate:

  • accounts.pb.ts → all the message definitions.
  • accountservice.gw.pb.ts → functions for calling AccountService gateway endpoints
  • adminservice.gw.pb.ts → functions for calling AdminService gateway endpoints

Additional Variant

As well as exposing simple functions, we could also introduce a more classical Client. The client instance could be used to store request configuration:

export class AccountServiceClient {
  constructor(initReq: InitReq) {
    this.initReq = initReq;
  } 

  createAccount(
    this: void,
    req: CreateAccountRequest
  ): Promise<CreateAccountResponse> {
    return createAccount(req, this.initReq);
  }
}
@dpup dpup added the enhancement New feature or request label May 16, 2024
@dpup
Copy link
Owner Author

dpup commented May 16, 2024

@seanami would be curious to hear your opinions on this.

@seanami
Copy link
Collaborator

seanami commented May 20, 2024

Nice! I like this as a way to make the code more idiomatic and also be able to have a more typical stateful client object that can preserve configuration across multiple requests.

I do use the service level class objects right now in some of my client configuration, but only to get the name of the service as a string for generating react-query cache names, and that can easily be replaced with something else.

I do highly suggest putting all of the request functions in per-service files. That will solve the potential dupe issue, and also allow those who want similar semantics as the existing implementation to do: import { * as AccountService } from ‘file’

@dpup
Copy link
Owner Author

dpup commented May 20, 2024

Any thoughts on naming of the files? Given accounts.proto should it be accounts-accountservice.gw.ts or just accountservice.gw.ts.

@seanami
Copy link
Collaborator

seanami commented May 20, 2024

Can service names be duplicated between separate proto files that are part of the same environment/project? E.g. Could I also have an additions.proto file in the same project which also defines an AccountService? (And if so, do the definitions get merged or what?)

If no duplicates are allowed, or the duplicates get merged into one definition, then I think just accountservice.gw.ts makes sense. Otherwise, I'd go with accounts-accountservice.gw.ts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants