Skip to content

Refactor ClientConfigBuilder.build#1498

Merged
cbrzn merged 13 commits intopileks/refactor-client-updatesfrom
pileks/refactor-client-updates-config-refactor
Jan 31, 2023
Merged

Refactor ClientConfigBuilder.build#1498
cbrzn merged 13 commits intopileks/refactor-client-updatesfrom
pileks/refactor-client-updates-config-refactor

Conversation

@pileks
Copy link
Copy Markdown
Contributor

@pileks pileks commented Jan 28, 2023

This PR aims to refactor out all unnecessary elements of the client-config-builder package, and defined what I saw as a "legacy" config for the PolywrapClient.

Generally inspired by @krisbitney's comments on #1480 with some additional cleanup.

Remaining work:

  • Figure out tests for the new ClientConfigBuilder now that it only outputs CoreClientConfig.

@pileks pileks changed the title Re Refactor ClientConfigBuilder.build Jan 28, 2023
@pileks pileks marked this pull request as draft January 28, 2023 19:39
@pileks pileks changed the base branch from origin-dev to pileks/refactor-client-updates January 28, 2023 19:40
Copy link
Copy Markdown
Contributor

@cbrzn cbrzn left a comment

Choose a reason for hiding this comment

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

I am a bit confused with the legacy folder, at first, I guess this is temporary, but how the long-term solution would look like?

);
expect(clientConfig.resolvers).toStrictEqual([testUriResolver]);

// TODO: How to test resolver?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not test that the resolver works here but that it can be added/removed and the object will be as expected. The resolver itself can be tested in the client (If I understand things correctly)

Copy link
Copy Markdown
Contributor Author

@pileks pileks Jan 30, 2023

Choose a reason for hiding this comment

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

Yup, that's my conclusion as well. However, CoreClientConfig only has an opaque resolver which cannot be tested structurally with something like shouldEqual.
I'm approaching this now by having the ClientConfigBuilder be able to "spit out" its _config object so that its structure can be tested.

@pileks
Copy link
Copy Markdown
Contributor Author

pileks commented Jan 30, 2023

I am a bit confused with the legacy folder, at first, I guess this is temporary, but how the long-term solution would look like?

I'm honestly not sure. It's not "legacy" per se, but it's there to support all the various forms of client config objects that we had in the past. The "non-legacy" approach is simply using ClientConfigBuilder, but allowing only that would break existing implementations of the PolywrapClient. I've therefore decided to bundle all of that up in a legacy folder so that we're aware that it's something that used to be done that way.

@pileks pileks marked this pull request as ready for review January 30, 2023 16:13
@pileks pileks requested a review from cbrzn January 30, 2023 18:01
@cbrzn cbrzn merged commit bc8ee1d into pileks/refactor-client-updates Jan 31, 2023
@dOrgJelli dOrgJelli deleted the pileks/refactor-client-updates-config-refactor 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.

2 participants