refactor(client): reduce config library surface #2269

fortuna · 2024-11-12T01:36:34Z

This PR further reduces the exposed surface of the config library and the dependency of config-specific details, advancing the goal to be config agnostic and allow for new protocols, including Websocket. It follows #2265 and many other previous changes.

In this PR I introduce the concepts of Dynamic and Static ServiceConfig. The parsing function now returns the firstHop in the TunnelConfigJson.

I'm trying to minimize the number of functions we need to implement in Go. It's a huge pain to expose even a single function from Go, since it requires touching all platforms. Electron is particularly bad, as functions are implemented as binary execution in a new process.

I believe I've reached a point where I only need to implement parseTunnelConfig in Go:

parseAccessKey is now a thin wrapper around parseTunnelConfig. It can stay that way.
validateAccessKey simply calls parseAccessKey
getHostFromTransportConfig is deleted
getAddressFromTransportConfig is only called from the parseTunnelConfig call tree, so it can move to Go.
setTransportConfigHost can be deleted if, for Electron, we resolve the first hop in parseTunnelConfig, and inject it in the returned config. Then the Electron code won't need to do the injection.

Phew! It took many attempts and iterations, but I believe this finally brings me to a place where we can move the config logic to Go!

daniellacosse

generally looks good, just some clarifying questions before I approve

client/src/www/app/outline_server_repository/config.ts

daniellacosse · 2024-11-13T20:48:26Z

client/src/www/app/outline_server_repository/server.ts

-          this.staticTunnelConfig.transport
-        );
+    const serviceConfig = parseAccessKey(accessKey);
+    this.name = name ?? serviceConfig.name;


will this enable people to dynamically set their service name? if so, do we want that?

It won't. The name comes directly from the fragment in the access key, not the Tunnel Config (which is what you fetch for dynamic keys).

Ahh got it.

daniellacosse · 2024-11-13T20:54:51Z

client/src/www/app/outline_server_repository/config.ts

+      config.method.data || 'unknown'
+    );
+  }
+  const transport: TransportConfigJson = {


I'm mildly confused now about what types should or should not be given the Json suffix. Is it data that's meant to be consumed by go and opaque to typescript? Or data that's meant to be sent over the network? Not exactly sure

The Json objects are those that must be serialized/deserialized into JSON. They are exchanged with the native code and must not have logic. They are simply a serialization format.

jyyi1

This is a huge leap forward! Thanks.

jyyi1 · 2024-11-13T20:47:06Z

client/src/www/app/outline_server_repository/config.ts

+export type ServiceConfig = {
+  readonly name: string;
+} & (StaticServiceConfig | DynamicServiceConfig);


Since name is common in StaticServiceConfig and DynamicServiceConfig, can we simplify the definition here to export type ServiceConfig = (StaticServiceConfig | DynamicServiceConfig)?

client/src/www/app/outline_server_repository/config.ts

jyyi1 · 2024-11-13T21:16:44Z

client/src/www/app/outline_server_repository/server.ts

+      this.tunnelConfigLocation = serviceConfig.transportConfigLocation;
+      this.displayAddress = '';
+
+      if (!serviceConfig.name) {


Will we override user provided name here?

Suggested change

if (!serviceConfig.name) {

if (!this.name) {

Good catch. Fixed.

client/src/www/app/outline_server_repository/index.ts

fortuna

Comments addressed

client/src/www/app/outline_server_repository/config.ts

fortuna · 2024-11-13T22:30:34Z

client/src/www/app/outline_server_repository/config.ts

+export type ServiceConfig = {
+  readonly name: string;
+} & (StaticServiceConfig | DynamicServiceConfig);


client/src/www/app/outline_server_repository/config.ts

client/src/www/app/outline_server_repository/index.ts

fortuna · 2024-11-13T22:36:14Z

client/src/www/app/outline_server_repository/server.ts

-          this.staticTunnelConfig.transport
-        );
+    const serviceConfig = parseAccessKey(accessKey);
+    this.name = name ?? serviceConfig.name;


It won't. The name comes directly from the fragment in the access key, not the Tunnel Config (which is what you fetch for dynamic keys).

fortuna · 2024-11-13T22:38:42Z

client/src/www/app/outline_server_repository/server.ts

+      this.tunnelConfigLocation = serviceConfig.transportConfigLocation;
+      this.displayAddress = '';
+
+      if (!serviceConfig.name) {


Good catch. Fixed.

fortuna · 2024-11-13T22:45:15Z

client/src/www/app/outline_server_repository/config.ts

+      config.method.data || 'unknown'
+    );
+  }
+  const transport: TransportConfigJson = {


The Json objects are those that must be serialized/deserialized into JSON. They are exchanged with the native code and must not have logic. They are simply a serialization format.

jyyi1

LGTM!

refactor(client): reduce config library surface

9bcee8a

fortuna requested review from daniellacosse and jyyi1 November 12, 2024 01:36

fortuna requested a review from a team as a code owner November 12, 2024 01:36

github-actions bot added the size/L label Nov 12, 2024

fortuna added 2 commits November 11, 2024 20:57

Lint fix

6587f1e

Merge branch 'master' into fortuna-clean

3513daa

daniellacosse reviewed Nov 13, 2024

View reviewed changes

jyyi1 reviewed Nov 13, 2024

View reviewed changes

fortuna added 4 commits November 13, 2024 17:22

Rename tunnelconfig

0be6ae4

Add comments

edbb365

Fix name

3ba7913

Address comments

6e351fd

fortuna commented Nov 13, 2024

View reviewed changes

fortuna requested review from daniellacosse and jyyi1 November 13, 2024 22:47

Fix add server

68e1df0

jyyi1 approved these changes Nov 13, 2024

View reviewed changes

fortuna added 2 commits November 13, 2024 18:29

Fix

f1814aa

Fix lint

8453247

fortuna enabled auto-merge (squash) November 13, 2024 23:48

daniellacosse approved these changes Nov 14, 2024

View reviewed changes

fortuna merged commit 49bf053 into master Nov 14, 2024
23 checks passed

fortuna deleted the fortuna-clean branch November 14, 2024 16:10

fortuna mentioned this pull request Nov 14, 2024

Add support for Websocket endpoints #2272

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(client): reduce config library surface #2269

refactor(client): reduce config library surface #2269

fortuna commented Nov 12, 2024 •

edited

Loading

daniellacosse left a comment

daniellacosse Nov 13, 2024

fortuna Nov 13, 2024

daniellacosse Nov 14, 2024

daniellacosse Nov 13, 2024

fortuna Nov 13, 2024

jyyi1 left a comment

jyyi1 Nov 13, 2024

fortuna Nov 13, 2024

jyyi1 Nov 13, 2024

fortuna Nov 13, 2024

fortuna left a comment

fortuna Nov 13, 2024

fortuna Nov 13, 2024

fortuna Nov 13, 2024

fortuna Nov 13, 2024

jyyi1 left a comment

refactor(client): reduce config library surface #2269

refactor(client): reduce config library surface #2269

Conversation

fortuna commented Nov 12, 2024 • edited Loading

daniellacosse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 left a comment

Choose a reason for hiding this comment

fortuna commented Nov 12, 2024 •

edited

Loading