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
Fix config typing and make Client easier to test #4711
Conversation
Client and ClientManager deal with both 'dehydrated' channels/networks (ie. directly from JSON configuration) and the 'rehydrated' ones (classes, with socket objects, message arrays, etc.). However, because their attributes are similar, both types were used interchangeably, which becomes an issue when splitting Client's configuration loading into smaller methods.
It will make it easier to write tests for what used to be in the connect() method
server/client.ts
Outdated
@@ -96,7 +96,7 @@ class Client { | |||
[socketId: string]: {token: string; openChannel: number}; | |||
}; | |||
config!: UserConfig & { | |||
networks?: Network[]; | |||
networks?: NetworkConfig[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this networks
isn't specified in UserConfig
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it's because it's optional in the attribute because of:
Lines 182 to 184 in efd24fd
// Networks are stored directly in the client object | |
// We don't need to keep it in the config object | |
delete client.config.networks; |
but required as the constructor argument.
but I don't see the point of making it required. Fixed
Thank you! |
I recommend reviewing commits separately (and avoiding squash-merging), it should be easier to read