Skip to content

ClientConfigBuilder - move wrapperCache and resolver from constructor to buildCoreConfig#1484

Merged
Niraj-Kamdar merged 3 commits intopileks/refactor-client-updatesfrom
pileks/bugfix/client-config-builder-wrapper-cache
Jan 26, 2023
Merged

ClientConfigBuilder - move wrapperCache and resolver from constructor to buildCoreConfig#1484
Niraj-Kamdar merged 3 commits intopileks/refactor-client-updatesfrom
pileks/bugfix/client-config-builder-wrapper-cache

Conversation

@pileks
Copy link
Copy Markdown
Contributor

@pileks pileks commented Jan 17, 2023

This PR aims to move ClientConfigBuilder constructor arguments into the only place where they are actually used - the buildCoreConfig method.

Context from Discord:

@krisbitney mentioned:
I noticed the ClientConfigBuilder constructor accepts a IWrapperCache argument, but the cache is ignored when calling build(). It's only used for buildCoreConfig(). Could this be misleading?

I'm thinking someone could call build() and then not realize they need to put their wrapper cache in a PolywrapClientConfig.

// config in constructor will be lost
const clientConfig = new ClientConfigBuilder(new CustomWrapperCache())
  .add(...)
  .build();

// my client doesn't use my custom cache!
const wrongClient = new PolywrapClient(clientConfig);

// Ohhh, it's because I forgot this step
const polywrapClientConfig = {
  ...clientConfig
   wrapperCach: new CustomWrapperCache(),
}

// this client will use the custom cache
const rightClient = new PolywrapClient(polywrapClientConfig);

After discussion between @pileks and @nerfZael it was decided that this is likely the best approach.

@pileks pileks self-assigned this Jan 17, 2023
@pileks pileks requested a review from nerfZael as a code owner January 17, 2023 20:41
Copy link
Copy Markdown
Contributor

@krisbitney krisbitney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for doing this!

@cbrzn cbrzn force-pushed the pileks/bugfix/client-config-builder-wrapper-cache branch from f16ff4e to b6c0db2 Compare January 25, 2023 19:25
@Niraj-Kamdar Niraj-Kamdar merged commit b924069 into pileks/refactor-client-updates Jan 26, 2023
@dOrgJelli dOrgJelli deleted the pileks/bugfix/client-config-builder-wrapper-cache branch April 10, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants