cockpit-project / cockpit

Cockpit is a web-based graphical interface for servers.
http://www.cockpit-project.org/
GNU Lesser General Public License v2.1
11.2k stars 1.11k forks source link

pkg/lib: create cockpit.ts #20417

Closed allisonkarlitskaya closed 4 months ago

allisonkarlitskaya commented 6 months ago

Rename cockpit.js to cockpit-core.js. Import it to cockpit.ts and re-export it as the default export. This is the traditional cockpit API up to this point, and you can still access it via:

import cockpit from 'cockpit';

Take our type annotations from cockpit.d.ts and move them into cockpit.ts, changing their format to an interface which describes the cockpit object (our default export). This is actually a bit of a reduction in readability because it means that we can no longer keep the helper interface types beside the functions that use them, but it simplifies our setup (removing a duplicate tsc call, speeding up test/static-code a bit).

As a side-effect of this change, we're no longer treating cockpit as a namespace, which means that we need to take the types that we've been publicly exporting on that namespace and export them from cockpit.ts, as non-default exports. That means that instead of doing something like:

   import cockpit from 'cockpit';

   ... cockpit.JsonObject

it will look like

   import cockpit, { JsonObject } from 'cockpit';

   ... JsonObject

In general, we plan that, going forward, new API will be added in this way and made available via named exports. Eventually (in the very distant future) the default export will be deprecated.

This is 100% pure RFC at this point, and I don't anticipate that we'll land this soon, but I wanted to get it off of my disk. I don't think I like exporting all of the interfaces (like UserInfo) from 'cockpit' with names like UserInfo but I'm not sure what a better way might look like.

Everything else I'm fairly happy with....

martinpitt commented 6 months ago

import cockpit, { JsonObject } from 'cockpit';

This would be ugly, and I don't like that. It'd still be better to treat cockpit as a namespace/module, to not clutter the global namespace. Fortunately JS has a syntax for that, even though it looks a little bit weird:

import * as cockpit from 'cockpit';

cockpit.JsonObject ...

We actually use this syntax quite a lot. E.g. pkg/lib/packagekit.js is in the same boat, and we e.g. do

pkg/systemd/overview-cards/realmd.jsx:import * as packagekit from "packagekit.js";

This is especially awkward with e.g.

import cockpit, { JsonObject, EventSource, EventListener, EventMap, BasicError, UserInfo } from 'cockpit';

from the current version of this PR, as it collides with the official JS API. That part of namespace cluttering gets my :-1:. So most of the "noise" of this PR should/can then be reverted if we just fix the imports/exports hopefully?

Other than that, this approach seems fine to me. Thanks!

jelly commented 6 months ago

import cockpit, { JsonObject } from 'cockpit';

This would be ugly, and I don't like that. It'd still be better to treat cockpit as a namespace/module, to not clutter the global namespace. Fortunately JS has a syntax for that, even though it looks a little bit weird:

Cluttering as we currently do? Like not having window.cockpit available? Because I don't see how this new approach changes much in regard to:

cockpit
{manifests: {…}, channel: ƒ, event_target: ƒ, extend: ƒ, utf8_encoder: ƒ, …}
window.cockpit
{manifests: {…}, channel: ƒ, event_target: ƒ, extend: ƒ, utf8_encoder: ƒ, …}

Being available, even worse is that some folks might depend on window.cockpit being available, which would then break. And it is used, although too many false positives to really figure that out: https://github.com/search?q=window.cockpit&type=code

allisonkarlitskaya commented 5 months ago

This would be ugly, and I don't like that. It'd still be better to treat cockpit as a namespace/module, to not clutter the global namespace. Fortunately JS has a syntax for that, even though it looks a little bit weird:

If I understand you correctly, you're advocating changing nothing in cockpit.ts but changing the places that I use it to use the import * as cockpit syntax?

In that case, if I understand you correctly, the main symbol would be called cockpit.cockpit... is that what you intended, or did I misunderstand?

martinpitt commented 5 months ago

Ah, I see what you mean. I was refering to importing particular things like { JsonObject }, which clutters the namespace. cockpit.cockpit would be ugly as well of course (and require lots of code churn). Now I understand the problem, we can't simultaneously import a module and the old "all but an object" as cockpit.

Maybe two imports like

// default export, backwards compatible
import cockpit from 'cockpit'
// other exports, new API
import * as cockpit_api from 'cockpit'

?

allisonkarlitskaya commented 5 months ago

So, there's 51 things that are hung on the main cockpit object:

assert base64_decode base64_encode byte_array cache channel dbus defer drop_privileges event_target extend file format format_bits_per_sec format_bytes format_bytes_per_sec format_number gettext grid hidden hint http info jump kill language language_direction localStorage locale location logout manifests message metrics ngettext noop oops permission reject resolve script series sessionStorage spawn translate transport user utf8_decoder utf8_encoder variant when

(with the event mixin methods suspiciously absent for some reason...)

We could export each of those as a separate thing from cockpit.ts... most of them would be straight-forward. Others (like language, location, and the event listener stuff) would be more difficult...

allisonkarlitskaya commented 5 months ago

So there's one good example of somewhere I'd not like to have an existing API directly exported on the module: gettext(). The existing implementation takes a context as an optional first argument in a way that definitely needs to die. Its replacement won't have that. So this is a case where we definitely want:

import cockpit from 'cockpit';

cockpit.gettext()

and

import { gettext } from 'cockpit';

gettext()

to be different.

chriswiggins commented 5 months ago

Would like to add my 2 cents - I think you'll be hard-pressed to find lots of implementations where the destructuring syntax isn't used.

Importing the default export, and then subsequently importing specific types / modules seems to be the 'de-facto' way of doing things.

TLDR; I support the following syntax:

import cockpit, { JsonObject } from 'cockpit';
allisonkarlitskaya commented 4 months ago

It's pretty clear to me after #20666 that our way forward here is going to look different.