pkg/lib: Port various components to TypeScript #20648
Conversation
allisonkarlitskaya
force-pushed
the
monday-morning-typescript
branch
from
7f9d34b
to
f11b8cd
Compare
| @@ -67,12 +73,3 @@ export const KebabDropdown = ({ dropdownItems, position = "end", isDisabled = fa | |||
| </Dropdown> | |||
| ); | |||
| }; | |||
|
|
|||
| KebabDropdown.propTypes = { | |||
There was a problem hiding this comment.
Yay, that works for ... typescript but this is also something which happens runtime in React which we now lose. (And this only works for full typescript projects, which more and more seems like a mistake to port projects to typescript).
There was a problem hiding this comment.
I dropped propTypes annotations on previous ports of code to typescript. I'm open to leaving them.
There was a problem hiding this comment.
I know and might have even approved it, but it sounds sensible to keep it around for third party consumers and podman/machines. Does it hurt us to keep it around?
There was a problem hiding this comment.
#20652 gets to the crux of the issue here, I think. Maybe we should discuss this there.
I think we want to be checking our JavaScript with tsc, but not in strict mode. In that case, -machines and friends would indeed benefit from the types.
There was a problem hiding this comment.
Only if you use typescript, which we don't require. So proptypes can stay useful. I don't think I rely on them as they are runtime and a warning. But when I do spot such issues I usually fix it.
There was a problem hiding this comment.
I can bring it back.
allisonkarlitskaya
force-pushed
the
monday-morning-typescript
branch
2 times, most recently
from
b0204eb
to
3a88ccf
Compare
There was a problem hiding this comment.
Thanks, I resolved all my previous threads. Assuming this goes green, good enough for me. But please also wait for @jelly 's re-review, not sure how strongly he feels about propTypes. They still seem to make sense to me for projects which aren't 100% TS yet (i.e. all but -files 😁 )
... mostly stuff used by Cockpit Files. This was all fairly trivial. In `utils.tsx` most of the changes are adjusting the newer language features rather than adding typing. For <Dropdown> we drop the `props` property — it's unused.
allisonkarlitskaya
force-pushed
the
monday-morning-typescript
branch
from
3a88ccf
to
91cb80e
Compare
| if (window.debugging == "all" || window.debugging?.includes("style")) { | ||
| console.debug([`cockpit-dark-theme: ${document.documentElement.id}:`, ...arguments].join(" ")); | ||
| console.debug([`cockpit-dark-theme: ${document.documentElement.id}:`, ...args].join(" ")); |
There was a problem hiding this comment.
This added line is not executed by any test.
einmal mit alles, bitte. auf die hand. |
| @@ -21,7 +21,7 @@ import React, { useState } from 'react'; | |||
| import PropTypes from "prop-types"; | |||
|
|
|||
| import { MenuToggle } from "@patternfly/react-core/dist/esm/components/MenuToggle"; | |||
| import { Dropdown, DropdownList } from "@patternfly/react-core/dist/esm/components/Dropdown"; | |||
| import { Dropdown, DropdownList, DropdownPopperProps } from "@patternfly/react-core/dist/esm/components/Dropdown"; | |||
There was a problem hiding this comment.
Wondering if we want to use import type not sure if get is useful because the current form wont get shook? Or does it?
There was a problem hiding this comment.
I think import type only makes sense on files that you wouldn't already import for non-type reasons.
ie: If you already have an import then don't add an import type. But if you wouldn't otherwise have an import, and only want it for typing reasons, then import type is the one you want.
... mostly stuff used by Cockpit Files.
This was all fairly trivial. In
utils.tsxmost of the changes are adjusting the newer language features rather than adding typing.