refactor(client): more config cleanup and consolidation #2265
Conversation
fortuna
changed the title
| @@ -23,6 +22,7 @@ export class AddAccessKeyDialog extends LitElement { | |||
| ) => string; | |||
| @property({type: Boolean}) open: boolean; | |||
| @property({type: String}) accessKey: string = ''; | |||
| @property({type: Function}) isValidAccessKey: (accessKey: string) => boolean; | |||
There was a problem hiding this comment.
nit: usually since this is a function not a boolean i'd give it an "action-based" name like validateAccessKey or accessKeyValidator for clarity but up to you
There was a problem hiding this comment.
The result is a boolean though. I think it reads more clearly in tha call sites that way.
|
|
||
| import * as errors from '../../model/errors'; | ||
|
|
||
| export type TransportConfigJson = object; |
There was a problem hiding this comment.
i assume you just moved all this code w/o modifying, but maybe we make this type more explicit? would clean up some of the type casting below. maybe in a follow up pr
There was a problem hiding this comment.
The type definition is intended to belong in the Go code. That's why I made it a generic object here.
The Typescript code is supposed to treat it as a black box and not dig into it. Though it does in the instances below.
At least now it's clearer where we violate the abstraction and what we still need to fix for advanced configs.
There was a problem hiding this comment.
So in the future these methods would belong in go? maybe worth a comment
There was a problem hiding this comment.
I agree to add a comment.
| export type TransportConfigJson = object; | |
| /** TransportConfigJson is an opaque config format defined in Go, TypeScript should not rely on its fields. */ | |
| export type TransportConfigJson = object; |
|
@jyyi1 thoughts? |
|
|
||
| import * as errors from '../../model/errors'; | ||
|
|
||
| export type TransportConfigJson = object; |
There was a problem hiding this comment.
I agree to add a comment.
| export type TransportConfigJson = object; | |
| /** TransportConfigJson is an opaque config format defined in Go, TypeScript should not rely on its fields. */ | |
| export type TransportConfigJson = object; |
| * getAddressFromTransportConfig returns the address of the tunnel server, if there's a meaningful one. | ||
| * This is used to show the server address in the UI when connected. | ||
| */ | ||
| export function getAddressFromTransportConfig( |
There was a problem hiding this comment.
Add a TODO to remind us moving this logic to Go?
There was a problem hiding this comment.
The TODO is for most of this file. I think it's pointless to add TODOs everywhere here. Also this is in flux.
This PR further removes dependencies on the Shadowsocks format, and consolidates them, so it's clear what needs to be fixed. I have created the
config.tsfile to centralize all the config logic.One critical cleanup was the removal of Shadowsocks validation from the add access key view.