Fix some type errors in JavaScript #21452

martinpitt · 2024-12-19T11:41:50Z

This reduces the ignore list by 8, some low-hanging fruit and quite useful! (it found little bugs)

Includes lib: Make SimpleSelect and CheckboxSelect generic in the option value type #21454, rebase after that lands

martinpitt · 2024-12-19T13:56:50Z

I'll rebase this after #21454, to clean up metrics.jsx in a nicer way.

`getFilterLabelKey()` can return `undefined`. Fix onDeleteChipGroup() to not set an undefined filter, same as onDeleteChip. This is the last instance of TS2464 "A computed property name must be of type ...".

Avoid the `delete` operator. We don't define the types explicitly, so tsc complains about > TS2790: The operand of a 'delete' operator must be optional. This was also a bit hard to read -- `binary` is derived from `options.binary`, that code just translates our historic API impedance mismatch between cockpit.js (treating `binary` as a booelan) and the JSON bridge protocol (where `binary` is a string with the only allowed value "raw"). Move this closer together, document it, and write it in a more explicit form.

mvollmer

Thanks! I think I found a nicer fix for the DBusProxies issue.

mvollmer · 2024-12-20T08:34:01Z

pkg/lib/cockpit.js

@@ -1900,6 +1900,8 @@ function factory() {
            if (!options)
                options = { };
            ensure_cache();
+            // this isn't a real class and only internal API; cockpit.d.ts annotates the return value DBusProxies
+            // @ts-expect-error TS7009: 'new' expression, whose target lacks a construct signature, implicitly has an 'any' type.


So, I think the fundamental issue is that TypeScript doesn't recognize function DBusProxies as a constructor. Things like

function Foo(x) { this.x = x; } const f = new Foo();

normally just work.

However, this produces the exact error TS7009:

function Bar(x) { } const b = new Bar();

The difference is that Bar does not assign to this, and neither does DBusProxies. It uses Object.defineProperties, and TS doesn't look inside of that apparently.

The TS7009 error goes away when adding a bogus self.foo = 0 assignment inside DBusProxies. That is enough to make TS understand that it is a constructor.

I would prefer to do that. I think this will work:

self.client = client; self.iface = iface; self.path_namespace = path_namespace; let waits; self.wait = function(func) { if (func) waits.always(func); return waits; } Object.defineProperties(self, { client: { enumerable: false, writable: false }, iface: { enumerable: false, writable: false }, path_namespace: { enumerable: false, writable: false }, wait: { enumerable: false, writable: false } });

That is, we initialize the new object in the normal way, and only use Object.defineProperties to modify the "enumerable" and "writable" attributes of the properties.

Niice, thanks for figuring this out! Reworked accordingly, let's confirm with the bo[ts]s!

mvollmer · 2024-12-20T08:42:50Z

test/common/typecheck

@@ -53,7 +53,6 @@ ignored_codes = [
    "TS7005",   # Variable '*' implicitly has an 'any[]' type.
    "TS7006",   # Parameter '*' implicitly has an 'any' type.
    "TS7008",   # Member '*' implicitly has an 'any[]' type.
-    "TS7009",   # 'new' expression, whose target lacks a construct signature, implicitly has an 'any' type.


I think it's okay to remove this here as well. The problem is not the implicit "any", but that TS missed that something is a constructor, which is worth fixing even in relaxed code.

Right, with that new approach we can and should fix similar instances

Fix the last remaining instance of > error TS7009: 'new' expression, whose target lacks a construct signature, implicitly has an 'any' type. That was because the DBusProxies() function was not considered a class constructor, as it did not assign to `this`. So do that for its properties, and only use `defineProperties()` for declaring their read-onliness. Thanks Marius Vollmer for figuring this out!

We dropped jQuery many years ago. This test is hopelessly outdated. This gets rid of "TS2581: Cannot find name '$'".

cockpit.spawn() never fails with a bare "closed". There are no expected failures there, so just always warn about them.

Don't put the cache onto the function object, that confuses the type checker. Make it an internal global variable instead. Fixes > pkg/systemd/hwinfo.jsx(133,39): error TS7024: Function implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. (There is another instance of this error which the next commit will address).

This function calls itself recursively, so the type checker cannot infer its signature. Add one explicitly. Fixes > pkg/storaged/crypto/keyslots.jsx(405,44): error TS7024: Function implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. which is the last remaining instance of that error. So stop ignoring it.

cockpituous · 2024-12-20T10:51:18Z

pkg/apps/appstream.js

-                        console.warn(error);
-                    }
-                });
+                .catch(error => console.warn("watch-appstream.py failed:", error));


This added line is not executed by any test.

cockpituous · 2024-12-20T10:51:18Z

pkg/lib/cockpit.js

+        self.wait = function(func) {
+            if (func)
+                waits.always(func);
+            return waits;


These 4 added lines are not executed by any test.

cockpituous · 2024-12-20T10:51:19Z

pkg/systemd/services.jsx

@@ -790,7 +790,13 @@ const ServicesPageFilters = ({
    const onDeleteChipGroup = (typeLabel) => {
        const type = getFilterLabelKey(typeLabel);

-        setFilters({ ...filters, [type]: [] });
+        if (type)
+            setFilters({ ...filters, [type]: [] });


This added line is not executed by any test.

cockpituous · 2024-12-20T10:51:19Z

pkg/systemd/services.jsx

+            setFilters({
+                activeState: [],
+                fileState: []


These 3 added lines are not executed by any test.

mvollmer

Thanks! Great that it works.

martinpitt force-pushed the types branch from 8471028 to 63d2717 Compare December 19, 2024 12:25

martinpitt mentioned this pull request Dec 19, 2024

lib: Make SimpleSelect and CheckboxSelect generic in the option value type #21454

Merged

martinpitt added the blocked Don't land until something else happens first (see task list) label Dec 19, 2024

martinpitt added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Dec 19, 2024

martinpitt force-pushed the types branch from 63d2717 to e331e65 Compare December 19, 2024 13:57

martinpitt added 2 commits December 19, 2024 16:07

systemd: Avoid setting undefined filter

b8e3ce0

`getFilterLabelKey()` can return `undefined`. Fix onDeleteChipGroup() to not set an undefined filter, same as onDeleteChip. This is the last instance of TS2464 "A computed property name must be of type ...".

martinpitt removed blocked Don't land until something else happens first (see task list) no-test For doc/workflow changes, or experiments which don't need a full CI run, labels Dec 19, 2024

martinpitt force-pushed the types branch from e331e65 to 1e420c2 Compare December 19, 2024 15:08

martinpitt requested a review from mvollmer December 19, 2024 15:08

mvollmer requested changes Dec 20, 2024

View reviewed changes

martinpitt added 6 commits December 20, 2024 10:37

base1: Drop obsolete test-no-jquery.js

69250b4

We dropped jQuery many years ago. This test is hopelessly outdated. This gets rid of "TS2581: Cannot find name '$'".

apps: Fix watch-appstream.py error handling

a0f1fb5

cockpit.spawn() never fails with a bare "closed". There are no expected failures there, so just always warn about them.

shell: Declare two machines helper functions

f4a9b30

martinpitt force-pushed the types branch from 1e420c2 to f4a9b30 Compare December 20, 2024 09:38

martinpitt requested a review from mvollmer December 20, 2024 09:52

cockpituous reviewed Dec 20, 2024

View reviewed changes

mvollmer approved these changes Dec 20, 2024

View reviewed changes

mvollmer merged commit c3f9c47 into cockpit-project:main Dec 20, 2024
86 of 87 checks passed

martinpitt deleted the types branch December 20, 2024 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some type errors in JavaScript #21452

Fix some type errors in JavaScript #21452

martinpitt commented Dec 19, 2024 •

edited

Loading

martinpitt commented Dec 19, 2024

mvollmer left a comment

mvollmer Dec 20, 2024 •

edited

Loading

martinpitt Dec 20, 2024

mvollmer Dec 20, 2024

martinpitt Dec 20, 2024

cockpituous Dec 20, 2024

cockpituous Dec 20, 2024

cockpituous Dec 20, 2024

cockpituous Dec 20, 2024

mvollmer left a comment

Fix some type errors in JavaScript #21452

Fix some type errors in JavaScript #21452

Conversation

martinpitt commented Dec 19, 2024 • edited Loading

martinpitt commented Dec 19, 2024

mvollmer left a comment

Choose a reason for hiding this comment

mvollmer Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

martinpitt Dec 20, 2024

Choose a reason for hiding this comment

mvollmer Dec 20, 2024

Choose a reason for hiding this comment

martinpitt Dec 20, 2024

Choose a reason for hiding this comment

cockpituous Dec 20, 2024

Choose a reason for hiding this comment

cockpituous Dec 20, 2024

Choose a reason for hiding this comment

cockpituous Dec 20, 2024

Choose a reason for hiding this comment

cockpituous Dec 20, 2024

Choose a reason for hiding this comment

mvollmer left a comment

Choose a reason for hiding this comment

martinpitt commented Dec 19, 2024 •

edited

Loading

mvollmer Dec 20, 2024 •

edited

Loading