Experimental typescript support #19842

allisonkarlitskaya · 2024-01-15T10:55:15Z

I want to use typescript in an upcoming test page in the playground. Let's lay some foundations.

I ported hooks.js to typescript because:

I use it from my page
it wasn't too hard
proof of concept to make sure nothing breaks

I didn't port cockpit.js because:

that's gonna be a lot of work

In any case, if everything is working correctly, this shouldn't actually change anything.

For the record:

31M	node_modules/typescript
548K	node_modules/@types
32M	total

The @types includes are fairly harmless, but I'm a bit more worried about adding typescript itself. That's a ~7% increase in the size of our node_modules. We could alternatively use the version of typescript available in Fedora and make sure to install it somewhere (tasks container, dev depenency during package builds, whatever) to make sure test/static-code can run it in CI.

todo

~~esbuild transparently supports .ts, but make sure this works with a webpack project, too (cockpit-composer?)~~ we don't care about webpack anymore

jelly · 2024-01-15T15:48:11Z

This is interesting like mypy is :-)

Some general note/thoughts:

I would love to have some IDE integration working with typescript as I am silly and sometimes type binary : "raw" on cockpit.channel which supports no such option. For that typescript would be great! So I would applaud typing cockpit.js and pkg/lib. I am not sure yet how I feel about normal pages/components.

The current approach of the PR requires files to be whitelisted to be linted and to be typescript, this seems suboptimal. I would love to already start using the types available to use so what I did was:

checkJs: true

Ok that does nothing, because we allowlist files, so I added pkg/shell/superuser.jsx that gives ~ 300 errors due to strict: true. Remove that and it becomes more reasonable:

Some really interesting things:

This is not typed, but already shows a "bug"

pkg/shell/superuser.jsx:127:31 - error TS2345: Argument of type 'string' is not assignable to parameter of type 'SetStateAction<boolean>'.

127                     setMethod(ids[0]);
                                  ~~~~~~

PattternFly is typescript so we get interesting errors:

pkg/shell/superuser.jsx:233:16 - error TS2322: Type 'string' is not assignable to type '"danger" | ComponentType<any> | "success" | "warning" | "info" | "custom"'.                    [34/1288]

233                titleIconVariant={title_icon}
                   ~~~~~~~~~~~~~~~~

  node_modules/@patternfly/react-core/dist/esm/components/Modal/Modal.d.ts:67:5
    67     titleIconVariant?: 'success' | 'danger' | 'warning' | 'info' | 'custom' | React.ComponentType<any>;
           ~~~~~~~~~~~~~~~~

when something is half typed or is imported without typing typescript lang server is not happy which is a big blocker :(

pkg/shell/machines/machines.js:726:31 - error TS2339: Property 'dbus' does not exist on type 'typeof import("/home/jelle/projects/cockpit/typescript/pkg/lib/cockpit")'.

726         const proxy = cockpit.dbus(null, { bus: "internal" }).proxy("cockpit.Machines", "/machines");

pkg/shell/superuser.jsx:23:28 - error TS2307: Cannot find module 'dialogs.jsx' or its corresponding type declarations.

23 import { useDialogs } from "dialogs.jsx";

I would need to do a bit more investigation, but it would be neat if we can maybe start whitelisting directories for typescript, the import issue seems to mostly block that but maybe there is a way around it?

jelly · 2024-01-15T15:13:29Z

pkg/lib/hooks.ts

@@ -262,10 +266,10 @@ export function useObject(create, destroy, deps, comps) {

    destroy_ref.current = destroy;
    useEffect(() => {
-        return () => destroy_ref.current?.(ref.current);
+        return () => { destroy_ref.current?.(ref.current!) };


What's the bang needed for?

The destroy function takes a non-null pointer, but the current value is null-typed. The ! is an assertion which means "I promise this isn't null".

pkg/lib/hooks.ts

pkg/lib/cockpit.d.ts

allisonkarlitskaya · 2024-01-15T20:49:57Z

Here's a few changes after feedback (as well as fixing the problems identified above).

The main changes:

less explicit handling of files in pkg/lib (although I'm not super happy about the solution).
no need to list every file anymore — we do a glob
checking of JS is enabled by default, so opening an editor on a .js or .jsx file is bound to be noisy. You can help reduce the noise by contributing to type annotations. When we run tsc from test/static-code we disable --checkJs.

Now, running tsc takes ~18s and produces a ridiculous amount of errors. Opening a file in the editor only takes ~3s before getting useful feedback, though. And tsc --checkJs false (which we call from test/static-code) takes less than 2 seconds.

allisonkarlitskaya · 2024-01-16T10:07:06Z

It hasn't been spoken out loud yet, so I'll say it now: the eventual goal is to have cockpit.ts. It's going to take a while, but this is a good start to give us a framework in which we could think about making it happen.

jelly · 2024-01-16T12:42:13Z

It hasn't been spoken out loud yet, so I'll say it now: the eventual goal is to have cockpit.ts. It's going to take a while, but this is a good start to give us a framework in which we could think about making it happen.

I think the team agrees on this. See this comment of @martinpitt and btw someone did attempt some type hints in the past maybe it's usable?

jelly · 2024-01-16T14:06:12Z

From our meeting:

Webpack - still required, when do we drop it?
Cockpit-composer - still uses webpack, does that work out of the box with hooks.ts?

mvollmer

Looks harmless enough to me.

allisonkarlitskaya · 2024-01-26T16:44:51Z

Let's set this aside for now. cockpit-project/cockpit-files#211 is where we're going to do this for the time being, but we can easily come back to this PR later.

Add the typescript compiler to our devel dependencies and run it from `test/static-code`. We add an initial cockpit.d.ts and a tsconfig.json which will get `tsc` to report errors against any `.ts` or `.tsx` files.

Add some type annotations to make it compile with strict mode enabled.

martinpitt · 2024-04-10T17:02:51Z

tsconfig.json

+ * Our main build is via `esbuild` which can handle `.ts`, but doesn't do any
+ * checking on its own.  To typecheck, run `tsc`.
+ *
+ * Extended JSON — comments and trailing commas are good here.


Ugh, does that not support something more expressive, like yaml..

martinpitt

Some of this is still under debate, but as it's completely on the "lint" side for now, it remains fungible. So I'm fine with landing that, and changing it as we need it -- at least we get the bits into node_modules/ for easy experimentation.

Thanks!

martinpitt · 2024-04-10T17:03:20Z

tsconfig.json

+        ],
+        "noEmit": true,  // we only use `tsc` for type checking
+        "skipLibCheck": true,  // don't check node_modules
+	"strict": true,


I suppose that's still under debate? It wasn't at all clear to me after the meeting whether we should turn this on or off.

I think the conclusion was that we want strict mode for .ts and a non-strict mode for .js coming along in some future PR.

martinpitt · 2024-04-10T17:04:48Z

pkg/lib/hooks.ts

@@ -262,10 +273,10 @@ export function useObject(create, destroy, deps, comps) {

    destroy_ref.current = destroy;
    useEffect(() => {
-        return () => destroy_ref.current?.(ref.current);
+        return () => { destroy_ref.current?.(ref.current!) };


Why does it need to be wrapped into {} now? Does the current() call return something non-void?

I think the ! somehow caused syntactic problems if it wasn't in a block. I honestly don't remember: it was a long time ago.

allisonkarlitskaya requested review from jelly and mvollmer January 15, 2024 10:55

allisonkarlitskaya force-pushed the tsc branch from 03562c6 to a64c911 Compare January 15, 2024 11:29

allisonkarlitskaya marked this pull request as ready for review January 15, 2024 13:38

allisonkarlitskaya force-pushed the tsc branch from a64c911 to 940a4c9 Compare January 15, 2024 13:55

This was referenced Jan 15, 2024

WIP: Add TypeScript bindings for public base1.js APIs #13214

Closed

New fsinfo channel #19845

Merged

jelly reviewed Jan 15, 2024

View reviewed changes

pkg/lib/cockpit.d.ts Outdated Show resolved Hide resolved

jelly reviewed Jan 15, 2024

View reviewed changes

pkg/lib/cockpit.d.ts Outdated Show resolved Hide resolved

allisonkarlitskaya force-pushed the tsc branch 2 times, most recently from 6c7dc8f to 7a3b436 Compare January 15, 2024 20:47

mvollmer reviewed Jan 19, 2024

View reviewed changes

allisonkarlitskaya closed this Jan 26, 2024

allisonkarlitskaya reopened this Apr 10, 2024

allisonkarlitskaya added 2 commits April 10, 2024 11:08

test/static-code: add support for typescript

335d5d4

Add the typescript compiler to our devel dependencies and run it from `test/static-code`. We add an initial cockpit.d.ts and a tsconfig.json which will get `tsc` to report errors against any `.ts` or `.tsx` files.

pkg/lib: rename hooks.js to hooks.ts

2edd8b1

Add some type annotations to make it compile with strict mode enabled.

allisonkarlitskaya force-pushed the tsc branch from 7a3b436 to 2edd8b1 Compare April 10, 2024 09:17

allisonkarlitskaya requested a review from martinpitt April 10, 2024 16:18

martinpitt reviewed Apr 10, 2024

View reviewed changes

martinpitt approved these changes Apr 10, 2024

View reviewed changes

martinpitt requested review from mvollmer and jelly April 10, 2024 17:06

allisonkarlitskaya merged commit 3ec539a into cockpit-project:main Apr 10, 2024
73 of 74 checks passed

allisonkarlitskaya deleted the tsc branch April 10, 2024 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental typescript support #19842

Experimental typescript support #19842

allisonkarlitskaya commented Jan 15, 2024 •

edited

Loading

jelly commented Jan 15, 2024

jelly Jan 15, 2024

allisonkarlitskaya Jan 15, 2024

allisonkarlitskaya commented Jan 15, 2024

allisonkarlitskaya commented Jan 16, 2024

jelly commented Jan 16, 2024

jelly commented Jan 16, 2024

mvollmer left a comment

allisonkarlitskaya commented Jan 26, 2024

martinpitt Apr 10, 2024

martinpitt left a comment

martinpitt Apr 10, 2024

allisonkarlitskaya Apr 10, 2024

martinpitt Apr 10, 2024

allisonkarlitskaya Apr 10, 2024

Experimental typescript support #19842

Experimental typescript support #19842

Conversation

allisonkarlitskaya commented Jan 15, 2024 • edited Loading

todo

jelly commented Jan 15, 2024

jelly Jan 15, 2024

Choose a reason for hiding this comment

allisonkarlitskaya Jan 15, 2024

Choose a reason for hiding this comment

allisonkarlitskaya commented Jan 15, 2024

allisonkarlitskaya commented Jan 16, 2024

jelly commented Jan 16, 2024

jelly commented Jan 16, 2024

mvollmer left a comment

Choose a reason for hiding this comment

allisonkarlitskaya commented Jan 26, 2024

martinpitt Apr 10, 2024

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt Apr 10, 2024

Choose a reason for hiding this comment

allisonkarlitskaya Apr 10, 2024

Choose a reason for hiding this comment

martinpitt Apr 10, 2024

Choose a reason for hiding this comment

allisonkarlitskaya Apr 10, 2024

Choose a reason for hiding this comment

allisonkarlitskaya commented Jan 15, 2024 •

edited

Loading