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.

Sign up for GitHub

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

shell: Even more types #21425

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

shell: Even more types #21425

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97d27a5
shell: Improve manifest types
mvollmer Dec 11, 2024
de229c6
shell: Fully type "nav"
mvollmer Dec 11, 2024
203a0fa
shell: Fully type "topnav"
mvollmer Dec 11, 2024
b9239a0
shell: Check types in "shell-modals"
mvollmer Dec 11, 2024
35a2d1e
shell: Fix two bugs found by TypeScript
mvollmer Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions pkg/shell/machines/machines.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { EventSource, EventMap } from "cockpit";
import { EventSource, EventMap, JsonObject } from "cockpit";

export function generate_connection_string(user: string | null, port: string | null, addr: string) : string;
export function split_connection_string (conn_to: string) : { address: string, user?: string, port?: number };
Expand Down Expand Up @@ -27,9 +27,7 @@ export interface ManifestSection {
[name: string]: ManifestEntry;
}

export interface Manifest {
[section: string]: ManifestSection;
}
export type Manifest = JsonObject;

export interface Manifests {
[pkg: string]: Manifest;
Expand Down
22 changes: 18 additions & 4 deletions pkg/shell/util.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import cockpit from "cockpit";

import { ManifestKeyword, ManifestDocs, Manifest, Manifests, Machine } from "./machines/machines";
import { ManifestKeyword, ManifestDocs, ManifestSection, Manifest, Manifests, Machine } from "./machines/machines";

export interface Location {
host: string;
Expand Down Expand Up @@ -91,6 +91,16 @@ export interface ManifestItem {
keywords: ManifestKeyword[];
}

export interface ManifestParentSection {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... we could make some functions like manifest_parent_section(m: Manifest): ManifestParentSection which would ide all the "unsafe" JSON manipulations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole Manifest stuff should go to its own file, actually.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, helpers which do type validation? what would they do on errors then?

This feels simultaneously too strict (hard to handle errors at that level, as these are effectively user-provided files) and too specific (we parse JSON everywhere, and should generalize the mechanics).

Are you aware of the bridge's jsonutil? That may be a more adequate way to parse JSON with strict typing, and the API would then also include default values.

martinpitt marked this conversation as resolved.
Show resolved Hide resolved
component?: string;
docs?: ManifestDocs[];
}

export interface ShellManifest {
docs?: ManifestDocs[];
locales?: { [id: string]: string };
}

export class CompiledComponents {
manifests: Manifests;
items: { [path: string] : ManifestItem; } = { };
Expand All @@ -101,7 +111,8 @@ export class CompiledComponents {

load(section: string): void {
Object.entries(this.manifests).forEach(([name, manifest]) => {
Object.entries(manifest[section] || { }).forEach(([prop, info]) => {
const manifest_section = (manifest[section] || {}) as ManifestSection;
Object.entries(manifest_section).forEach(([prop, info]) => {
Comment on lines +114 to +115
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cast is weird and IMHO detrimental. This isn't a case of "ts can't figure it out", it's literally "this could be anything as it's an user-provided (or at least user-customizable) external file on disk". So even after the as you can in no way be sure that you actually have a ManifestSection object, and the type checker will hide errors such as "component is actually an int". I.e. you'll get runtime errors because you forgot to check the type. That's the opposite of what typing should achieve.

That's why I think for this case something like json_get_object(manifest, section, {}) would be much cleaner, more robust, and also more general here. The original code was actually fine already, but the helper functions could produce proper console warnings that help admins to figure out why their manifest customizations are being ignored.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cast is weird and IMHO detrimental.

Yes... it is keeping the status quo, but we should try to do better eventually, agreed.

I.e. you'll get runtime errors because you forgot to check the type. That's the opposite of what typing should achieve.

Hmm, I would call this input validation rather than static typing. Currently, we validate manifest input by just pretending it's all valid and let the shell crash when it isn't. (And we have been fine with that since forever.) Static typing makes this obvious, but by itself is not enough to improve it.

We would have to write new code to do real input validation. I was hoping there is a way to get TypeScript to do that. Here are some good pointers: https://stackoverflow.com/questions/33800497/check-if-an-object-implements-an-interface-at-runtime-with-typescript

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I would call this input validation rather than static typing

Yes, exactly. And input validation is a runtime thing, the type checker can't help with that. We either need some "JSON schema validation" thing, or just pick out the values individually with the expected type and default (which I personally favor -- it's explicit, tsc can help us get it right, and we don't reject manifests with wrong keys wholesale, which would be a behaviour changes).

const item: ManifestItem = {
path: "", // set below
hash: "", // set below
Expand Down Expand Up @@ -172,8 +183,11 @@ export class CompiledComponents {
// Still don't know where it comes from, check for parent
if (!component) {
const comp = this.manifests[path];
if (comp && comp.parent && comp.parent.component)
component = comp.parent.component as string;
if (comp && comp.parent) {
const parent = comp.parent as ManifestParentSection;
if (parent.component)
component = parent.component;
}
Comment on lines -175 to +190
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same spirit as above, I think componennt = comp?.parent?.component would be both cleaner and more robust. Alternatively, wrap this into some json_get_object() helper with warning messages -- i.e. "nonexisting" is fine (default null) but "exists but wrong type" should warn.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you are asking for "The Shell should validate the schema of all manifests". That's a good idea. We could have done that without TypeScript all these years, but we didn't. A bug in a manifests is like a bug in JavaScript, and should be found and fixed by the developer.

And while TypeScript might benefit from it once schema validation is done (think generating TS types from JSON schemas), I don't think TypeScript by itself is gonna help. All it does it point out our sins...

So, can I ask to keep the manifest schema validation yak out of this PR? :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, can I ask to keep the manifest schema validation yak out of this PR?

I am fine with that. I am not fine with pretending that everything is good here and papering over the issue with as -- it really isn't, and tsc rightfully complains. So if it's too hard to fix these places, then let's add something greppable and obvious like @ts-expect-error. (Because as does have valid use cases, and isn't greppable)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary from meeting: My gut feeling/slight preference is the get_json_string() kind of accessor on usage, but I agree that for common keys we may rather want some more centralized "schema validation" thing, with a constructor. My minimum requirement here would be to add a // HACK: unsafe "as" or similar so that we can find these again.

}

const item = this.items[component];
Expand Down