Config builder and CLI config refactor #1367
Conversation
nerfZael
requested review from
Niraj-Kamdar,
dOrgJelli,
krisbitney and
namesty
as code owners
| const idx = this.config.envs.findIndex((x) => Uri.equals(x.uri, envUri)); | ||
|
|
||
| if (idx > -1) { | ||
| this.config.envs.splice(idx, 1); | ||
| } |
There was a problem hiding this comment.
nit: wouldn't it be easier to just use filter here?
There was a problem hiding this comment.
i think this comment can be applied to all the remove* that uses this logic
There was a problem hiding this comment.
Haven't written this code, but I suspect it's a bit faster than using filter since the arrays are always unique.
I'm fine with either solution
cc @pileks
There was a problem hiding this comment.
In that case, having a Set would be the fastest. I belive the loading will also be easier
packages/test-cases/cases/cli/wasm/build-cmd/assemblyscript/001-sanity/yarn.lock
Outdated
Show resolved
Hide resolved
| const idx = this.config.envs.findIndex((x) => Uri.equals(x.uri, envUri)); | ||
|
|
||
| if (idx > -1) { | ||
| this.config.envs.splice(idx, 1); | ||
| } |
There was a problem hiding this comment.
In that case, having a Set would be the fastest. I belive the loading will also be easier
| return this; | ||
| } | ||
|
|
||
| addRedirect(from: Uri | string, to: Uri | string): ClientConfigBuilder { |
There was a problem hiding this comment.
What do you think about having redirects as a map instead of a list of from-to?
There was a problem hiding this comment.
We talked about making envs a map, but yea, redirects could also make sense. I think the same applies to wrappers and packages.
I'll look into the best way to do that in a follow-up PR
| addInterfaceImplementations( | ||
| interfaceUri: Uri | string, | ||
| implementationUris: Array<Uri | string> | ||
| ): ClientConfigBuilder { | ||
| const interfaceUriSanitized = Uri.from(interfaceUri); | ||
| const implementationUrisSanitized = implementationUris.map(Uri.from); |
There was a problem hiding this comment.
This can also represented as map to simplify things
Closes #1378