shell: TypeScript workflow demo #21265
Open
+407
−192
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mvollmer
force-pushed
the
shell-typing
branch
2 times, most recently
from
cf7aa37
to
d0ec28a
Compare
mvollmer
changed the title
|
state.tsx should cause a lot of typing errors, of which only one is interesting: |
mvollmer
force-pushed
the
shell-typing
branch
3 times, most recently
from
001b8a0
to
0aef6ae
Compare
mvollmer
added
the
no-test
mvollmer
force-pushed
the
shell-typing
branch
3 times, most recently
from
e85ea9b
to
605fbf1
Compare
mvollmer
changed the title
1 task
mvollmer
force-pushed
the
shell-typing
branch
3 times, most recently
from
008922d
to
a33cc13
Compare
Aha, and after rebasing this on top of #20826, we get one more: Well done, TypeScript! |
martinpitt
requested changes
Nov 17, 2024
There was a problem hiding this comment.
Thanks @mvollmer! I'm excited about the general direction. I have a few questions and suggestions.
mvollmer
force-pushed
the
shell-typing
branch
2 times, most recently
from
f9a1316
to
12fd679
Compare
mvollmer
force-pushed
the
shell-typing
branch
4 times, most recently
from
61d3f8b
to
c1bdb5b
Compare
|
Yes! :-) That's the point of this PR. We have to fix them. of course. What do you think of the rest now? |
mvollmer
removed
the
no-test
martinpitt
requested changes
Nov 27, 2024
mvollmer
force-pushed
the
shell-typing
branch
from
f126501
to
7a9d930
Compare
Instead of running tsc directly.
To reduce the complexity of the Shell slightly. The build_href function is special in that it constructs a string for a Location object without including the configured URL root of this Cockpit instance. The only point of this is to create strings that can be passed to ShellState.jump. They also show up in href attributes, but they never really should. So, let's never pass a string to ShellState.jump and just pass the Location object directly.
The conection string functions are moved to machines/machines so that util can use types from machines/machines without causing a dependency cycle. CompiledComponents has been rewritten as a proper class. This is the Right Thing(tm) for TypeScript.
The "state" file now participates in type checking, but we still allow the "any" type. TypeScript will automatically infer "any" in many places and shut up. Later we can add more types, which will mean a rewrite of ShellState as a proper class.
mvollmer
force-pushed
the
shell-typing
branch
from
7a9d930
to
a191244
Compare
martinpitt
approved these changes
Nov 28, 2024
There was a problem hiding this comment.
Thanks! This looks nice to me now, and I'm looking forward to reaping a lot more benefits from our pkg/lib and cockpit.js typing efforts. Thanks!
But @jelly may also have an opinion here, so I'd also let him review that if he wants to.
cockpituous
reviewed
Nov 28, 2024
Comment on lines
+148
to
+152
| <a className="pf-v5-c-nav__section-title nav-item" | ||
| href={encode_location(g.action.target)} | ||
| onClick={ ev => { | ||
| this.props.jump(g.action.target); | ||
| ev.preventDefault(); |
There was a problem hiding this comment.
These 5 added lines are not executed by any test.
Comment on lines
+154
to
+155
| {g.action.label} | ||
| </a> |
There was a problem hiding this comment.
These 2 added lines are not executed by any test.
| @@ -356,10 +363,10 @@ export const PageNav = ({ state }) => { | |||
| if (current_machine_manifest_items.items.apps && groups.length === 3) | |||
| groups[0].action = { | |||
| label: _("Edit"), | |||
| path: build_href({ | |||
| target: { | |||
There was a problem hiding this comment.
This added line is not executed by any test.
| host: current_machine.address, | ||
| path: current_machine_manifest_items.items.apps.path | ||
| }) | ||
| path: current_machine_manifest_items.items.apps.path, |
There was a problem hiding this comment.
This added line is not executed by any test.
| @@ -75,26 +79,23 @@ export function ShellState() { | |||
|
|
|||
| machines.addEventListener("ready", on_ready); | |||
|
|
|||
| machines.addEventListener("removed", (ev, machine) => { | |||
| machines.addEventListener("removed", (_, machine) => { | |||
There was a problem hiding this comment.
This added line is not executed by any test.
| export function encode_location(location: Location): string { | ||
| const shell_embedded = window.location.pathname.indexOf(".html") !== -1; | ||
| if (shell_embedded) | ||
| return window.location.toString(); |
There was a problem hiding this comment.
This added line is not executed by any test.
| section, | ||
| label: cockpit.gettext(info.label) || prop, | ||
| label: info.label ? cockpit.gettext(info.label) : prop, |
There was a problem hiding this comment.
This added line is not executed by any test.
| const compiled = new CompiledComponents(manifests); | ||
| compiled.load("tools"); | ||
| compiled.load("dashboard"); | ||
| compiled.load("menu"); | ||
| return compiled; | ||
| } | ||
|
|
||
| function component_checksum(machine, path) { | ||
| function component_checksum(machine: Machine, path: string): string | undefined { |
There was a problem hiding this comment.
This added line is not executed by any test.
|
@jelly gentle review ping? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is how I imagine working when bringing more TypeScript to Cockpit:
pkg/shell/util.tsx..tsortsxand made "relaxed clean" without thinking hard about that. The idea is that the types provided by the "strict clean" files will help find bugs in those hastily converted files. In this PR this isstate.tsxand we found two bugs in it already.ShellStateand gettingshell.jsxrelaxed-clean. (This is happening in theshell-typing-2branch, not here.)The idea is that I can do some typing work (in non-trivial files) that I find relevant and not too scary to take on, and then merge something that I know will be kept from regressing.