test: port test-format to typescript

[allisonkarlitskaya]

This was mostly pain-free, except that it tests a very wide range of
possible values to the format functions. Expand the type hints
accordingly.

There was one case that required a bit of a nudge — deepEqual wants both
sides to be the same type, and one of them is very static in nature and
the other quite dynamic — just use as unknown to force it down.

[jelly]

I want to veto this, this is a terrible API. We should not type how we don't want code to be written.

Passing factor as MB and then reverse looking up that it was 1000 is awful imo and we should rid ourselves of it.

[allisonkarlitskaya]

I can accept this argument, but it leaves us in an interesting place: we test this behaviour, but want to make it statically impossible...

[allisonkarlitskaya]

One way we can do this is with as any casts in the tests... not sure how I feel about it...

[jelly]

My believe is that typing should help us uncover weird things, like the superuser: true and we should then fix this

[allisonkarlitskaya]

Let's chat about this tomorrow. There's definitely a conversation to be had here...

[allisonkarlitskaya]

Just to be clear, "get rid of" could mean:

• completely remove this functionality from cockpit.js, breaking the API
• remove the advertisement of this functionality from cockpit.d.ts, breaking only typescript users
• /** @deprecate */ the functionality in cockpit.d.ts
• something else?

[martinpitt]

I'd like to actually break the API, given that there are zero known users; but I'm too scared about the unknown ones. So, my gut feeling:

• Design the API as we want it (see my proposal above, I'm happy to discuss of course)
• Do the types exactly for this, so that we learn about unintended API usage via type checks
• Then add some backwards compat hacks inside of format_bytes() to opportunistically interpret a non-object second argument: if it's "1024", treat it as { binary: true }, and ignore all others. console.warn() about it in any case.

Is that possible/reasonable?

[allisonkarlitskaya]

So here's one proposal that I think you might like, which would let us keep something like the existing podman case:

    /* === Number formatting ===================== */

    interface FormatFactor { _tag: "format-factor" }
    type FormatOptions = {
        output?: "value" | "unit";
        precision?: number;
        base2?: boolean;
        factor?: FormatFactor;
    };

    function format_number(n?: number?, precision?: number): string
    function choose_format_factor(n?: number): FormatFactor;
    function format_bytes(n?: number?, options?: FormatOptions): string;
    function format_bytes_per_sec(n?: number?, options?: FormatOptions): string;
    function format_bits_per_sec(n?: number?, options?: FormatOptions): string;

and here's a simplified version that makes the podman case impossible:

    /* === Number formatting ===================== */

    type FormatOptions = {
        precision?: number;
        base2?: boolean;
    };

    function format_number(n?: number?, precision?: number): string
    function format_bytes(n?: number?, options?: FormatOptions): string;
    function format_bytes_per_sec(n?: number?, options?: FormatOptions): string;
    function format_bits_per_sec(n?: number?, options?: FormatOptions): string;

I'd be OK with either.

[martinpitt]

TBH I don't understand how the first proposal would work for "the podman case", by which I suppose you mean "uses the separate option" -- it will return a string[] then (or even before cockpit-project/cockpit-podman#1672 it would get a boolean third argument).

Also, for the second one, options is really an object ({ precision: 3 }), whereas in the first one the {} feel more like the grouping for the various types that the second argument could have? (bool or int or string). This doesn't seem consistent?

(That said, I really dislike the current API; bool | int | string is just crazy)

[allisonkarlitskaya]

The idea with the more complicated API is that you can use choose_format_factor(size) to get an opaque FormatFactor object which can then be passed as factor to multiple invocations of format_bytes() to force the same units from each invocation. You could then opt out of the display of the units with output: "value";

It was just an idea. I think I'd actually prefer not to do it.

[allisonkarlitskaya]

I've split out the hopefully-less-controversial parts of this to separate PRs.

[martinpitt]

FTR: There are zero uses of this outside of cockpit.git, and it's not documented. The only place that uses it is in cockpit/main/pkg/storaged/dialog.jsx . Let's make that private?

[allisonkarlitskaya]

→ #20334