Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/lib: create cockpit.ts #20417

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

allisonkarlitskaya
Copy link
Member

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....

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.
@martinpitt
Copy link
Member

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 👎. 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!

@martinpitt martinpitt removed their request for review May 3, 2024 04:11
@jelly
Copy link
Member

jelly commented May 3, 2024

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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';

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants