feat: configuration service for LogLevel and chain ws address

[LeonFLK]

fixes KILTProtocol/ticket#774

This merges and replaces ConfigLog.ts.
We do not want to connect implicitly to the default ws address, instead set it directly or via Kilt.config to set the address of our desired node. This address can be changed any time.
LogLevel can still be set by ENV.
This non-strongly typed approach uses generic set and get to manipulate and access configOpts configuration object.
No specific key suggestion anymore since configOpts is union type with generic keyed object.
Throws error on unset generic property key when accessing or on retrieval of unset address.

Checklist:

• I have verified that the code works
• I have verified that the code is easy to understand
  • If not, I have left a well-balanced amount of inline comments
• I have left the code in a better state
• I have documented the changes (where applicable)

[tjwelde]

I think from the API it would be best, if I could call Kilt.config({...}).

[rflechtner]

I'd prefer a config service that does not rely on a fixed set of fields to be able to extend easily in the future. What would you say, @tjwelde ?
I also remember we talked about config via environment variables, what's your opinion on this now?

[LeonFLK]

Now works much more under the hood.
Use Kilt.config() to pass partial configOpts (address and logLevel).
If no host address is set, getting the address will result in thrown error.
getCached and similar all use configuration.host get for retrieval of set address.

[rflechtner]

As I had mentioned when we talked about it, I am not a fan of hard-coding specific configuration options into the config service. I predict this will be a nightmare for maintaining our codebase later. Also it doesn't work well with the project to split the SDK into smaller sub-packages. Are we sure we want to go down that road ?

To make for some more constructive feedback, I had imagined something like this:

let configuration: configOpts = {'logLevel': DEFAULT_DEBUG_LEVEL}

export function get<K extends keyof configOpts>(configOpt: K): configOpts[K] {
  if (typeof configuration[configOpt] === 'undefined') 
  throw new Error(`GENERIC NOT CONFIGURED ERROR FOR ${configOpt}`)
    return configuration[configOpt]
}

export function set(opts: Partial<configOpts>): void {
  if(typeof opts['logLevel'] !== 'undefined') {
    opts.logLevel = modifyLogLevel(opts.logLevel)
  }
  configuration = {...configuration, ...opts}
}

which could be exported as ConfigService or Kilt.config and would allow users to set options with (for example) Kilt.config.set({defaultNodeAddress: 'value'}) and get values with Kilt.config.get('defaultNodeAddress').

[tjwelde]

Just two nitpicks, otherwise I think it looks pretty good.
What do you think @rflechtner

[rflechtner]

Just two nitpicks, otherwise I think it looks pretty good.
What do you think @rflechtner

Looks good & works for me!

All resolved

[rflechtner]

Sure, but it saves you the scrolling. I think that could help quite a bit to make it obvious what’s happening.

-- From: LeonFLK <notifications@github.com> Reply to: KILTprotocol/sdk-js <reply@reply.github.com> Date: Wednesday, 27. January 2021 at 15:04 To: KILTprotocol/sdk-js <sdk-js@noreply.github.com> Cc: Raphael Flechtner <mail@dasraph.de>, Mention <mention@noreply.github.com> Subject: Re: [KILTprotocol/sdk-js] feat: configuration service for LogLevel and chain ws address (#312) @LeonFLK commented on this pull request. In packages/core/src/blockchainApiConnection/BlockchainApiConnection.ts:

@@ -31,7 +29,7 @@ export const CUSTOM_TYPES: RegistryTypes = {

}  export async function buildConnection( -  host: string = DEFAULT_WS_ADDRESS +  host: string | undefined = get('address') For me this wouldn't change comprehension, since we already import get from ConfigService, this is unnecessary IMO. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[LeonFLK]

I mean one can always ctrl+click to get to the definition, and that specific file is quite small, so not much scrolling going on.

Just think if we were to clarify each import like that and hold it to the same standard it's going to be messy.

What do you think, make an exception for this file clarifying the import, or skip?

Edit: import { get as ConfigService.get } does not work, would you want to import the whole ConfigService and then use the different functions like you said?

[rflechtner]

yeah importing the ConfigService as a whole would we the solution. But it's not a must, leave it if you don't think it adds much

[tjwelde]

Uff, finally, right? ;)
LGTM 🎉