fix: add BuildOptions to build method in IClientConfigBuilder

[krisbitney]

This makes it possible to add a custom cache or resolver without casting.

Before:

const builder: IClientConfigBuilder = new ClientConfigBuilder().addInterface(...);
const config = (builder as ClientConfigBuilder).build(resolver);

After:

const builder: IClientConfigBuilder = new ClientConfigBuilder().addInterface(...);
const config = builder.build(resolver);

[Niraj-Kamdar]

LGTM!

[cbrzn]

i thought about this, and I am not sure if we want this. the reason is that (imo) we don't want the interface to be aware of our implementation details. If someone wants to implement its builder, he probably needs to implement the build method with our options (or with no options at all).

for example (iiuc), this piece of code will throw error

export class MyBuilder extends BaseClientConfigBuilder {
  addDefaults(): IClientConfigBuilder {
    return this;
  }
  build(_options?: { custom: string }): CoreClientConfig {
    return {} as CoreClientConfig;
  }
}

edit: I might be wrong tho but just wanted to share my thoughts 😄

[krisbitney]

i thought about this, and I am not sure if we want this. the reason is that (imo) we don't want the interface to be aware of our implementation details. If someone wants to implement its builder, he probably needs to implement the build method with our options (or with no options at all).

for example (iiuc), this piece of code will throw error

export class MyBuilder extends BaseClientConfigBuilder {
  addDefaults(): IClientConfigBuilder {
    return this;
  }
  build(_options?: { custom: string }): CoreClientConfig {
    return {} as CoreClientConfig;
  }
}

edit: I might be wrong tho but just wanted to share my thoughts 😄

I think this is a bad UX. I thought it was a bug. Maybe we should reconsider how we add a custom cache or custom resolver?

[cbrzn]

I think this is a bad UX. I thought it was a bug. Maybe we should reconsider how we add a custom cache or custom resolver?

yup, I think this is a code smell that is indicating us that