JS client: change envs and interfaces into readonly maps#1557
Conversation
pileks
changed the base branch from
origin-dev
to
pileks/feat/workflow-manifest-0.2.0
…s-client-config-unique-maps
…2.0' into pileks/feat/js-client-config-unique-maps
…client-config-unique-maps
…client-config-unique-maps
pileks
changed the base branch from
pileks/feat/workflow-manifest-0.2.0
to
origin-dev
…s-client-config-unique-maps
…client-config-unique-maps
pileks
requested review from
cbrzn,
dOrgJelli,
krisbitney,
namesty and
nerfZael
as code owners
pileks
changed the title
envs and interfaces into readonly maps
krisbitney
left a comment
krisbitney
left a comment
There was a problem hiding this comment.
Everything looks good. This is well done! The use of objects instead of arrays looks really clean.
Before merging, I think the question about using string vs. Uri should be resolved, just because of the recent decision to use Uri in the core.
I think making the implementations array of type Uri[] instead of string[] makes sense.
| describe.each(supportedImplementations)( | ||
| "client <-> wrappers end to end", | ||
| (i) => { | ||
| typeTestCases(i); |
There was a problem hiding this comment.
Oops, need to revert this
| * | ||
| * @returns an array of interfaces and their registered implementations | ||
| */ | ||
| getInterfaces(): readonly InterfaceImplementations[] | undefined; |
There was a problem hiding this comment.
I noticed some inconsistencies in this file when it comes to readonly and ReadOnly<...> usage. Could we please use it the same in all methods / properties? If I'm missing some nuances please inform :)
…client-config-unique-maps
| } | ||
|
|
||
| return envs.find((environment) => Uri.equals(environment.uri, uriUri)); | ||
| return envs[uri.uri]; |
There was a problem hiding this comment.
This will probably treat "wrap://ens/" and "ens/" as different URIs
There was a problem hiding this comment.
We could maybe sanitize this stuff in the constructor
4 participants
Closes #1283
This PR aims to change the
envsandinterfacescollections insideCoreClientConfiginto readonly unique maps/map-like structures.envsis now essentially:interfacesis now essentially:Everything that can be is marked as
readonlyin order to relay the intended readonly nature of the CoreClientConfig.Some comments/feedback needed:
strings, but implementing some customURI-Mapseems like a worse idea. This does leave us vulnerable to people mixingwrap://xyz/...andxyz/...as two separate objects within the maps, but that's something that should be handled by the ClientConfigBuilder.ClientConfigBuilderis not always used yet, and I know some devs want to simply build their own config.interfacesbeing aRecord<string, Uri[]>instead. Thoughts?