new Channel API #20731

allisonkarlitskaya · 2024-07-09T15:31:41Z

This brings along a new approach to typing Channel which should make it easier to implement derived channel types.

martinpitt

The first 5 commits are unrelated/independent. Sending a separate review for these.

pkg/lib/cockpit/_internal/transport.js

martinpitt

Thanks, this looks nice! I have a few nitpicks, but mostly questions.

pkg/lib/cockpit/_internal/transport.d.ts

pkg/lib/cockpit/fsinfo.ts

pkg/lib/cockpit/channel.ts

martinpitt · 2024-07-11T15:43:08Z

pkg/lib/cockpit/channel.ts

+            this.#received[command] = control;
+            this.emit(command, control);
+        } else {
+            this.emit('control', control);


I checked a lot of the umpteen uncovered code complaints. Many are really unreachable "should not happen" code, and things like this are because we just ported over one channel type. This should get covered if we e.g. port the file channel to this (anything that supports a binary channel).

pkg/lib/cockpit/channel.ts

allisonkarlitskaya · 2024-07-16T16:22:40Z

So the new warnings about 'send messages after close' are showing that we sometimes call .close() on closed channels...

I wonder if we should try to fix that at the use site (I guess fsinfo?) or try to be more tolerant in this particular case...

martinpitt · 2024-07-17T13:16:36Z

I'm almost 💯 sure that this is a flake, but let's retry to compare.

martinpitt

Thanks! There's only a bunch of questions and small stuff left. This grew so big, I hope the next iteration can be "it". 😁

pkg/lib/cockpit/_internal/transport.js

pkg/lib/cockpit/_internal/parentwebsocket.ts

pkg/lib/cockpit/_internal/transport.js

pkg/base1/test-channel.ts

pkg/lib/cockpit/channel.ts

martinpitt · 2024-07-17T20:56:55Z

Hmm, these unit tests hanging on exactly all Debian/Ubuntus sounds too much for a coincidence 😢

martinpitt · 2024-07-17T20:58:09Z

Wait -- these tests fail in exactly the same way in #20772 -- did we break main somehow?

martinpitt · 2024-07-17T21:01:14Z

Last landed PR #20763 from 8 hours ago had green tests. This happens during .deb package build, so it can't access the internet even. So what on earth changed!?

allisonkarlitskaya · 2024-07-17T21:29:44Z

Last landed PR #20763 from 8 hours ago had green tests. This happens during .deb package build, so it can't access the internet even. So what on earth changed!?

Could be a case of landing two PRs in parallel, each tested against their common ancestor, but never tested together until they hit main.

allisonkarlitskaya · 2024-07-17T23:22:50Z

I'm gonna assume this has something to do with #20768... it's acting like the systemd mainloop isn't being properly registered.

allisonkarlitskaya · 2024-07-17T23:24:00Z

...and indeed we landed #20768 without a full slate because I didn't think pytest ran during image builds.

allisonkarlitskaya · 2024-07-18T00:35:58Z

→ #20773

- disable the base no-use-before-define rule: it doesn't understand that TypeScript allows using types before they're defined. Enable the TypeScript-specific rule for it for TypeScript files. - add an 'unused variables' config matching the default rules of the TypeScript compiler Both of these rule stanzas were copied from the documentation of the typescript-eslint plugin. Links to those pages are provided in comments, which are valid in the extended JSON syntax used for eslintrc. Tweak our test/static-code rule for JSON to also exempt eslintrc from our strict checking.

Having these in the "global" namespace alongside everything in node_modules seems like a disaster waiting to happen. Let's make a `cockpit` dir and import them like `'cockpit/fsinfo'`, etc.

Stop using array_from_raw_string() and join_data() to create binary frames and replace it with open-coded uses of `TextEncoder.encode()` and `Uint8Array.set()`. Drop the now-unused `array_from_raw_string()`. Tweak the type check for binary messages to 'anything not a string'.

There are two APIs in pkg/lib/_internal/transport.js that aren't used in any way outside of that file. Don't "export" them.

Take the offer of the TypeScript compiler to "Convert function to an ES2015 class". This was a completely automated operation.

This class defines a single event, and the only code that connects to it lives inside of the same file. We can move this to our new API quite easily.

Avoiding `function` means that `this` is the same thing throughout the entire constructor function. Change a couple of `window` event listeners while we're at it.

We no longer need this alias for `this` — since we only use arrows inside of the constructor, we can access `this` directly.

Currently all of the functionality of this class is defined inside of the constructor. We want to start moving some of it out to separate functions, so move all of the private state to instance private properties.

These methods no longer depend on anything inside of the constructor() function and can be moved out. This is safe because there are no call sites that use these functions as callbacks, so `this` will always be correctly bound. The methods formerly exported on the `self` object are turned into public methods and the others are turned into privates with `#`. The one tricky case here is `dispatch_data()` which is now a public method, but also used as the WebSocket `.onmessage` handler. In that case we use an arrow to make sure `this` is properly provided: this.#ws.onmessage = event => this.dispatch_data(event);

Use a spread operator instead. Also let's us avoid using `.apply()`.

We can safely assume by now that the browser always supports WebSocket. This lets us eliminate a fallback path and clean up the logic a bit.

This essentially follows all the steps that we did with Transport, but is small enough to do it in a single go.

Instead of having the transport code reach in to the global `cockpit` object to set properties, just have the `init_callback` (already part of the `cockpit` code) do it for us.

"in" filtering is used by the shell to inspect all incoming messages for forwarding to the appropriate frame, but nothing uses "out" filters, outside of a single test case which exists only to test the feature. Removing this will substantially simplify the code and allow for further cleanups, so let's drop it.

Now that we dropped support for filters, this only has a single call site, and the types it has to deal with are somewhat simplified. Merge it directly into that site, combining the logic for reading the channel name with the logic for splitting out the payload part of the message.

Instead of setting this to null by default, make it an empty list. That way we can avoid the check for initializing it, and can do a simple iteration, unconditionally.

Instead of checking for truthiness, check that the value is actually a string before assigning it to a variable that's used as a string.

This seems like a bit of a strange thing to do, but it will make it easier to add annotations when porting to TypeScript. Annotating object literals requires providing a type separate from the value, whereas a class allows the annotations to be given inline.

After all the prep work, this is a reasonably straight-forward exercise.

This is the API that we intend all future channel implementations (including users outside of cockpit.js) to use. Port fsinfo.

martinpitt

Rebased after the pytest fix, cheers! This should go green now. 🚀

cockpituous · 2024-07-18T04:17:02Z

pkg/lib/cockpit.js

-                if (!transport_globals.outgoing_filters)
-                    transport_globals.outgoing_filters = [];
-                transport_globals.outgoing_filters.push(callback);
+                console.error("'out' filters are no longer supported");


This added line is not executed by any test.

cockpituous · 2024-07-18T04:17:02Z

pkg/lib/cockpit/_internal/common.ts

@@ -33,7 +33,7 @@ if (typeof window.debugging === "undefined") {
            get: function() { return window.sessionStorage.debugging || window.localStorage.debugging },
            set: function(x) { window.sessionStorage.debugging = x }
        });
-    } catch (e) { }
+    } catch { }


This added line is not executed by any test.

cockpituous · 2024-07-18T04:17:02Z

pkg/lib/cockpit/_internal/location.ts

@@ -7,7 +7,7 @@ function get_url_root(): string | null {
        try {
            // Sometimes this throws a SecurityError such as during testing
            return window.localStorage.getItem('url-root');
-        } catch (e) {
+        } catch {


This added line is not executed by any test.

cockpituous · 2024-07-18T04:17:02Z

pkg/lib/cockpit/_internal/parentwebsocket.ts

+    readyState = 0;
+
+    // essentially signal handlers: these are assigned to from Transport
+    onopen(): void {


This added line is not executed by any test.

cockpituous · 2024-07-18T04:17:03Z

pkg/lib/cockpit/_internal/parentwebsocket.ts

+    onopen(): void {
+    }
+
+    onclose(): void {


This added line is not executed by any test.

cockpituous · 2024-07-18T04:17:04Z

pkg/lib/cockpit/channel.ts

+            if (this.#received.ready) {
+                resolve(this.#received.ready);
+            } else if (this.#received.close) {
+                reject(this.#received.close);


These 4 added lines are not executed by any test.

cockpituous · 2024-07-18T04:17:05Z

pkg/lib/cockpit/channel.ts

+                this.on('ready', resolve);
+                this.on('close', reject);


These 2 added lines are not executed by any test.

cockpituous · 2024-07-18T04:17:05Z

pkg/lib/cockpit/channel.ts

+    toString(): string {
+        const state =
+            (!this.id && 'waiting for transport') ||
+            (this.#received.close?.problem && `${this.id} error ${this.#received.close.problem}`) ||
+            (this.#received.close && `${this.id} closed`) ||
+            (this.#received.ready && `${this.id} opened`) ||
+            `${this.id} waiting for open`;


These 7 added lines are not executed by any test.

cockpituous · 2024-07-18T04:17:05Z

pkg/lib/cockpit/channel.ts

+            (this.#received.ready && `${this.id} opened`) ||
+            `${this.id} waiting for open`;
+
+        const host = this.options?.host || "localhost";


This added line is not executed by any test.

cockpituous · 2024-07-18T04:17:05Z

pkg/lib/cockpit/channel.ts

+
+        const host = this.options?.host || "localhost";
+
+        return `[Channel ${state} -> ${this.options?.payload}@${host}]`;


This added line is not executed by any test.

allisonkarlitskaya requested a review from martinpitt July 9, 2024 15:31

allisonkarlitskaya force-pushed the channel-api branch from 5a1804e to 272a1c0 Compare July 9, 2024 16:38

martinpitt reviewed Jul 11, 2024

View reviewed changes

pkg/lib/cockpit/_internal/transport.js Outdated Show resolved Hide resolved

martinpitt reviewed Jul 11, 2024

View reviewed changes

allisonkarlitskaya force-pushed the channel-api branch 6 times, most recently from dea421a to a1fefe1 Compare July 16, 2024 02:00

allisonkarlitskaya mentioned this pull request Jul 16, 2024

add coverage output for qunit tests #20767

Merged

allisonkarlitskaya force-pushed the channel-api branch 5 times, most recently from 48bb540 to b68c2a5 Compare July 16, 2024 16:17

allisonkarlitskaya force-pushed the channel-api branch 2 times, most recently from af30ac5 to 1263b0b Compare July 16, 2024 17:31

allisonkarlitskaya requested a review from martinpitt July 17, 2024 13:04

martinpitt requested changes Jul 17, 2024

View reviewed changes

allisonkarlitskaya force-pushed the channel-api branch from 1263b0b to b390fd1 Compare July 17, 2024 15:46

allisonkarlitskaya requested a review from martinpitt July 17, 2024 15:49

martinpitt mentioned this pull request Jul 17, 2024

lib: isInline and isLiveRegion are wrongly typed #20772

Merged

allisonkarlitskaya added 22 commits July 18, 2024 05:25

pkg/lib: move event/fsinfo/_internal into cockpit/

dc4c524

Having these in the "global" namespace alongside everything in node_modules seems like a disaster waiting to happen. Let's make a `cockpit` dir and import them like `'cockpit/fsinfo'`, etc.

pkg/lib: un-"export" Transport API

733af26

There are two APIs in pkg/lib/_internal/transport.js that aren't used in any way outside of that file. Don't "export" them.

pkg/lib: transport: Convert function to a class

b199774

Take the offer of the TypeScript compiler to "Convert function to an ES2015 class". This was a completely automated operation.

pkg/lib: use EventEmitter on Transport API

33a209c

This class defines a single event, and the only code that connects to it lives inside of the same file. We can move this to our new API quite easily.

pkg/lib: use arrows in Transport.constructor()

dc1f037

Avoiding `function` means that `this` is the same thing throughout the entire constructor function. Change a couple of `window` event listeners while we're at it.

pkg/lib: drop self in Transport.constructor

f7f84eb

We no longer need this alias for `this` — since we only use arrows inside of the constructor, we can access `this` directly.

pkg/lib: convert Transport.constructor locals to privates

b786762

Currently all of the functionality of this class is defined inside of the constructor. We want to start moving some of it out to separate functions, so move all of the private state to instance private properties.

pkg/lib: avoid arguments in transport.js

8378b3a

Use a spread operator instead. Also let's us avoid using `.apply()`.

pkg/lib: simplify WebSocket setup code

bd694cb

We can safely assume by now that the browser always supports WebSocket. This lets us eliminate a fallback path and clean up the logic a bit.

pkg/lib: convert ParentWebSocket to class

4d8615e

This essentially follows all the steps that we did with Transport, but is small enough to do it in a single go.

pkg/lib: port ParentWebSocket to TypeScript

acb708b

pkg/lib: remove public_transport hook

9bc7d24

Instead of having the transport code reach in to the global `cockpit` object to set properties, just have the `init_callback` (already part of the `cockpit` code) do it for us.

pkg/lib: simplify incoming filter logic

3134ad6

Instead of setting this to null by default, make it an empty list. That way we can avoid the check for initializing it, and can do a simple iteration, unconditionally.

pkg/lib: step up type checking in Transport

b0d7dc3

Instead of checking for truthiness, check that the value is actually a string before assigning it to a variable that's used as a string.

pkg/lib: port Transport to TypeScript

b966834

After all the prep work, this is a reasonably straight-forward exercise.

pkg/lib: add new public "Channel" API

2d0fda8

This is the API that we intend all future channel implementations (including users outside of cockpit.js) to use. Port fsinfo.

martinpitt force-pushed the channel-api branch from b390fd1 to 2d0fda8 Compare July 18, 2024 03:26

martinpitt approved these changes Jul 18, 2024

View reviewed changes

cockpituous reviewed Jul 18, 2024

View reviewed changes

martinpitt merged commit d4003a0 into cockpit-project:main Jul 18, 2024
76 of 77 checks passed

martinpitt deleted the channel-api branch July 18, 2024 04:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new Channel API #20731

new Channel API #20731

allisonkarlitskaya commented Jul 9, 2024 •

edited

Loading

martinpitt left a comment

martinpitt left a comment

martinpitt Jul 11, 2024

allisonkarlitskaya commented Jul 16, 2024

martinpitt commented Jul 17, 2024

martinpitt left a comment

martinpitt commented Jul 17, 2024

martinpitt commented Jul 17, 2024

martinpitt commented Jul 17, 2024

allisonkarlitskaya commented Jul 17, 2024

allisonkarlitskaya commented Jul 17, 2024

allisonkarlitskaya commented Jul 17, 2024

allisonkarlitskaya commented Jul 18, 2024

martinpitt left a comment

cockpituous Jul 18, 2024

cockpituous Jul 18, 2024

cockpituous Jul 18, 2024

cockpituous Jul 18, 2024

cockpituous Jul 18, 2024

cockpituous Jul 18, 2024

cockpituous Jul 18, 2024

cockpituous Jul 18, 2024

cockpituous Jul 18, 2024

cockpituous Jul 18, 2024


		const host = this.options?.host \|\| "localhost";

		return `[Channel ${state} -> ${this.options?.payload}@${host}]`;

new Channel API #20731

new Channel API #20731

Conversation

allisonkarlitskaya commented Jul 9, 2024 • edited Loading

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonkarlitskaya commented Jul 16, 2024

martinpitt commented Jul 17, 2024

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt commented Jul 17, 2024

martinpitt commented Jul 17, 2024

martinpitt commented Jul 17, 2024

allisonkarlitskaya commented Jul 17, 2024

allisonkarlitskaya commented Jul 17, 2024

allisonkarlitskaya commented Jul 17, 2024

allisonkarlitskaya commented Jul 18, 2024

martinpitt 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

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

allisonkarlitskaya commented Jul 9, 2024 •

edited

Loading