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

Jump to bottom

storage: Some typing fun #21200

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

storage: Some typing fun #21200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions pkg/lib/cockpit.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@ declare module 'cockpit' {
href: string;
go(path: Location | string, options?: { [key: string]: string }): void;
replace(path: Location | string, options?: { [key: string]: string }): void;

encode(path: string | Array<string>,
options? : { [name: string]: string | Array<string> },
Copy link
Member

Choose a reason for hiding this comment

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

You can also write Record<> for that, I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just copy/pasting... :-)

with_root?: boolean);
}

export const location: Location;
Expand Down
76 changes: 49 additions & 27 deletions pkg/storaged/pages.jsx → pkg/storaged/pages.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,8 @@ const _ = cockpit.gettext;
complication of how cards and pages interact.
*/

let pages = null;
let crossrefs = null;
let pages: Map<string, any> = new Map();
let crossrefs: Map<string, any> = new Map();

export const PAGE_CATEGORY_PHYSICAL = 1;
export const PAGE_CATEGORY_VIRTUAL = 2;
Expand Down Expand Up @@ -181,7 +181,7 @@ function size_from_card(card) {
return size_from_card(card.next);
}

export function new_page(parent, card, options) {
export function new_page(parent, card, options = {}) {
const page = {
location: location_from_card(card),
name: name_from_card(card),
Expand All @@ -191,13 +191,13 @@ export function new_page(parent, card, options) {
parent,
children: [],
card,
options: options || {},
options,
columns: [
card.title,
card.location,
size_from_card(card),
]
};
page.columns = [
card.title,
card.location,
size_from_card(card),
];
if (parent)
parent.children.push(page);
while (card) {
Expand Down Expand Up @@ -225,6 +225,26 @@ export function new_card({
has_warning, has_danger, job_path,
component, props,
actions,
} : {
title?,
location?,
next?,
type_extra?,
id_extra?,
page_name?,
page_icon?,
page_category?,
page_key?,
page_location?,
page_size?,
page_block?,
for_summary?,
has_warning?,
has_danger?,
job_path?,
component?,
props?,
actions?,
}) {
if (page_block) {
page_location = [block_location(page_block)];
Expand Down Expand Up @@ -317,7 +337,7 @@ function make_menu_item(action) {
}

function make_page_kebab(page) {
const items = [];
const items: React.JSX.Element[] = [];

function card_item_group(card) {
const a = card.actions || [];
Expand Down Expand Up @@ -372,8 +392,8 @@ const ActionButtons = ({ card }) => {
return false;
}

const buttons = [];
const items = [];
const buttons: React.JSX.Element[] = [];
const items: React.JSX.Element[] = [];

if (!card.actions)
return null;
Expand All @@ -396,7 +416,7 @@ const ActionButtons = ({ card }) => {
};

function page_type_extra(page) {
const extra = [];
const extra: any[] = [];
let c = page.card;
while (c) {
if (c.type_extra)
Expand Down Expand Up @@ -434,8 +454,8 @@ function page_type(page) {
// Thus, we end up with things like "Partition - MDRAID device".

function page_block_summary_1(page) {
let description = null;
const extra = [];
let description = "";
const extra: any[] = [];
for (let card = page.card; card; card = card.next) {
if (card.for_summary) {
description = card.id_extra || card.title;
Expand All @@ -460,9 +480,9 @@ function page_block_summary(page) {
return desc1 || desc2;
}

let narrow_query = null;
let narrow_query: any = null;

export const useIsNarrow = (onChange) => {
export const useIsNarrow = (onChange?) => {
if (!narrow_query) {
const val = window.getComputedStyle(window.document.body).getPropertyValue("--pf-v5-global--breakpoint--md");
narrow_query = window.matchMedia(`(max-width: ${val})`);
Expand All @@ -472,12 +492,13 @@ export const useIsNarrow = (onChange) => {
return narrow_query.matches;
};

export const PageTable = ({ emptyCaption, aria_label, pages, crossrefs, sorted, show_icons }) => {
export const PageTable = ({ emptyCaption, aria_label, pages, crossrefs, sorted, show_icons } :
{ emptyCaption, aria_label, pages?, crossrefs?, sorted, show_icons }) => {
const [collapsed, setCollapsed] = useState(true);
const firstKeys = useRef(false);
const firstKeys : any = useRef(false);
const narrow = useIsNarrow(() => { firstKeys.current = false });

let rows = [];
let rows: React.JSX.Element[] = [];
const row_keys = new Set();

function make_row(page, crossref, level, border, key) {
Expand All @@ -502,7 +523,7 @@ export const PageTable = ({ emptyCaption, aria_label, pages, crossrefs, sorted,
return false;
}

let info = null;
let info: React.JSX.Element | null = null;
if (card_has_job(page.card))
info = <>{"\n"}<Spinner isInline size="md" /></>;
if (card_has_danger(page.card))
Expand Down Expand Up @@ -609,7 +630,7 @@ export const PageTable = ({ emptyCaption, aria_label, pages, crossrefs, sorted,
} else {
const cols = [
<Td key="1" onClick={onClick}>
<div className="indent" style={ { "--level": level } }>
<div className="indent" style={ { "--level": level } as React.CSSProperties }>
<Truncate content={name} />
{info}
</div>
Expand Down Expand Up @@ -688,7 +709,7 @@ export const PageTable = ({ emptyCaption, aria_label, pages, crossrefs, sorted,
</EmptyState>;
}

let show_all_button = null;
let show_all_button: React.JSX.Element | null = null;
if (rows.length > 50 && collapsed) {
show_all_button = (
<Bullseye>
Expand Down Expand Up @@ -756,7 +777,7 @@ function page_display_name(page) {
}

const PageCardStackItems = ({ page, plot_state }) => {
const items = [];
const items: React.JSX.Element[] = [];
let c = page.card;
while (c) {
items.push(<React.Fragment key={items.length}>
Expand All @@ -775,7 +796,7 @@ export function block_location(block) {
}

const StorageBreadcrumb = ({ page }) => {
const parent_crumbs = [];
const parent_crumbs: React.JSX.Element[] = [];
let pp = page.parent;
while (pp) {
parent_crumbs.unshift(
Expand All @@ -793,7 +814,8 @@ const StorageBreadcrumb = ({ page }) => {
</Breadcrumb>);
};

export const StorageCard = ({ card, alert, alerts, actions, children }) => {
export const StorageCard = ({ card, alert, alerts, actions, children } :
{ card, alert?, alerts?, actions?, children }) => {
return (
<Card data-test-card-title={card.title}>
{ (client.in_anaconda_mode() && card.page.parent && !card.next) &&
Expand Down Expand Up @@ -847,7 +869,7 @@ export const StoragePage = ({ location, plot_state }) => {
<PageSection isFilled={false} padding={client.in_anaconda_mode() ? { default: "noPadding" } : {}}>
<Stack hasGutter>
<MultipathAlert client={client} />
<PageCardStackItems page={page} plot_state={plot_state} noarrow />
<PageCardStackItems page={page} plot_state={plot_state} />
</Stack>
</PageSection>
</Page>
Expand Down
105 changes: 49 additions & 56 deletions pkg/storaged/storage-controls.jsx → pkg/storaged/storage-controls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,28 +47,26 @@ const _ = cockpit.gettext;
* excuse in a tooltip.
*/

class StorageControl extends React.Component {
render() {
const excuse = this.props.excuse;
if (!client.superuser.allowed)
return <div />;
const StorageControl = ({ excuse, excuse_placement, content } :
{ excuse, excuse_placement?, content }) => {
if (!client.superuser.allowed)
return <div />;
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.


if (excuse) {
return (
<Tooltip id="tip-storage" content={excuse}
position={this.props.excuse_placement || TooltipPosition.top}>
<span>
{ this.props.content(excuse) }
</span>
if (excuse) {
return (
<Tooltip id="tip-storage" content={excuse}
position={excuse_placement || TooltipPosition.top}>
<span>
{ content(excuse) }
</span>
</Tooltip>
);
} else {
return this.props.content();
}
);
} else {
return content();
}
}
};

function checked(callback, setSpinning, excuse) {
function checked(callback, setSpinning?, excuse?) {
return function (event) {
if (!event)
return;
Expand Down Expand Up @@ -110,7 +108,8 @@ function checked(callback, setSpinning, excuse) {
};
}

export const StorageButton = ({ id, kind, excuse, onClick, children, ariaLabel, spinner }) => {
export const StorageButton = ({ id, kind, excuse, onClick, children, ariaLabel, spinner } :
{ id?, kind, excuse, onClick, children, ariaLabel?, spinner? }) => {
const [spinning, setSpinning] = useState(false);

return <StorageControl excuse={excuse}
Expand All @@ -120,13 +119,13 @@ export const StorageButton = ({ id, kind, excuse, onClick, children, ariaLabel,
onClick={checked(onClick, setSpinning)}
variant={kind || "secondary"}
isDisabled={!!excuse || (spinner && spinning)}
isLoading={spinner ? spinning : undefined}>
isLoading={spinner ? spinning : false}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

{children}
</Button>
)} />;
};

export const StorageLink = ({ id, excuse, onClick, children }) => (
export const StorageLink = ({ excuse, onClick, children }) => (
<StorageControl excuse={excuse}
content={excuse => (
<Button onClick={checked(onClick)}
Expand All @@ -141,43 +140,36 @@ export const StorageLink = ({ id, excuse, onClick, children }) => (
// StorageOnOff - OnOff switch for asynchronous actions.
//

export class StorageOnOff extends React.Component {
constructor() {
super();
this.state = { promise: null };
}
export const StorageOnOff = ({ excuse, "aria-label": aria_label, state, onChange }) => {
const [promise, setPromise] = useState(null);
const [promise_goal_state, setPromiseGoalState] = useState(null);

render() {
const self = this;

function onChange(_event, val) {
const promise = self.props.onChange(val);
if (promise) {
promise.catch(error => {
dialog_open({
Title: _("Error"),
Body: error.toString()
});
})
.finally(() => { self.setState({ promise: null }) });
}

self.setState({ promise, promise_goal_state: val });
function _onChange(_event, val) {
const promise = onChange(val);
if (promise) {
promise.catch(error => {
dialog_open({
Title: _("Error"),
Body: error.toString()
Comment on lines +150 to +153
Copy link
Contributor

Choose a reason for hiding this comment

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

These 4 added lines are not executed by any test.

});
})
.finally(() => { setPromise(null) });
}

return (
<StorageControl excuse={this.props.excuse}
content={(excuse) => (
<Switch isChecked={this.state.promise
? this.state.promise_goal_state
: this.props.state}
aria-label={this.props['aria-label']}
isDisabled={!!(excuse || this.state.promise)}
onChange={onChange} />
)} />
);
setPromise(promise);
setPromiseGoalState(val);
}
}

return (
<StorageControl excuse={excuse}
content={(excuse) => (
<Switch isChecked={promise ? promise_goal_state : state}
aria-label={aria_label}
isDisabled={!!(excuse || promise)}
onChange={_onChange} />
)} />
);
};

/* Render a usage bar showing props.stats[0] out of props.stats[1]
* bytes in use. If the ratio is above props.critical, the bar will be
Expand All @@ -200,7 +192,7 @@ export const StorageUsageBar = ({ stats, critical, block, offset, total, short }
</span>
<div className={"usage-bar" + (fraction > critical ? " usage-bar-danger" : "") + (short ? " usage-bar-short" : "")}
role="progressbar"
aria-valuemin="0" aria-valuemax={stats[1]} aria-valuenow={stats[0]}
aria-valuemin={0} aria-valuemax={stats[1]} aria-valuenow={stats[0]}
aria-label={cockpit.format(_("Usage of $0"), block)}
aria-valuetext={labelText}>
<div className="usage-bar-indicator usage-bar-other" aria-hidden="true" style={{ width: total_fraction * 100 + "%" }} />
Expand Down Expand Up @@ -232,7 +224,8 @@ export const StorageMenuItem = ({ onClick, danger, excuse, children }) => (
</DropdownItem>
);

export const StorageBarMenu = ({ label, isKebab, menuItems }) => {
export const StorageBarMenu = ({ label, isKebab, menuItems } :
{ label?, isKebab, menuItems }) => {
const [isOpen, setIsOpen] = useState(false);

if (!client.superuser.allowed)
Expand Down
1 change: 1 addition & 0 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"moduleResolution": "node",
"noEmit": true, // we only use `tsc` for type checking
"strict": true,
"noImplicitAny": false,
Copy link
Member

Choose a reason for hiding this comment

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

Can you please split this bit out as a separate commit with some verbiage about the impact and trade-offs? Does this apply to *.ts as well? (which we should keep strict), or just to *.js (then that'd be nice)

Copy link
Member

Choose a reason for hiding this comment

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

NACK from me on that one. We definitely don't want to lose this checking in test/static-code.

You could change this in your editor's LSP config, or if we decide that changing it here is indeed the best way, we need to re-enable it again from the tsc run in test/static-code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you please split this bit out as a separate commit with some verbiage about the impact and trade-offs?

I hope we don't need to do this globally.

Does this apply to *.ts as well? (which we should keep strict), or just to *.js (then that'd be nice)

It applies to all files, unfortunately. I'll see how to switch this off for individual files.

Copy link
Member

Choose a reason for hiding this comment

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

A setting for individual files would be OK. Some kind of comment at the top or so?

"target": "es2020",
},
"include": [
Expand Down
Loading