storage: Some typing fun #21200
storage: Some typing fun #21200
Conversation
mvollmer
force-pushed
the
storage-typing-fun
branch
from
b6fd6d7
to
6965d37
Compare
|
Somehow TS doesn't allow this: TS infers "never[]" as the type for "items" and doesn't allow pushing strings onto it. But @allisonkarlitskaya says this should work and TS should infer "string[]" as the type, and it does in her experiments. |
| @@ -23,6 +23,7 @@ | |||
| "moduleResolution": "node", | |||
| "noEmit": true, // we only use `tsc` for type checking | |||
| "strict": true, | |||
| "noImplicitAny": false, | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
A setting for individual files would be OK. Some kind of comment at the top or so?
| @@ -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> }, | |||
There was a problem hiding this comment.
You can also write Record<> for that, I think?
There was a problem hiding this comment.
I was just copy/pasting... :-)
| @@ -23,6 +23,7 @@ | |||
| "moduleResolution": "node", | |||
| "noEmit": true, // we only use `tsc` for type checking | |||
| "strict": true, | |||
| "noImplicitAny": false, | |||
There was a problem hiding this comment.
A setting for individual files would be OK. Some kind of comment at the top or so?
mvollmer
added
the
no-test
| const StorageControl = ({ excuse, excuse_placement, content } : | ||
| { excuse, excuse_placement?, content }) => { | ||
| if (!client.superuser.allowed) | ||
| return <div />; |
There was a problem hiding this comment.
This added line is not executed by any test.
| @@ -120,13 +119,13 @@ | |||
| onClick={checked(onClick, setSpinning)} | |||
| variant={kind || "secondary"} | |||
| isDisabled={!!excuse || (spinner && spinning)} | |||
| isLoading={spinner ? spinning : undefined}> | |||
| isLoading={spinner ? spinning : false}> | |||
There was a problem hiding this comment.
This added line is not executed by any test.
| promise.catch(error => { | ||
| dialog_open({ | ||
| Title: _("Error"), | ||
| Body: error.toString() |
There was a problem hiding this comment.
These 4 added lines are not executed by any test.
This is caused by the version of "tsc". I had 5.1.3 installed from Fedora repos, which shows this behavior. I also have node_modules/.bin/tsc which is 5,6,3, and infers "any[]"... |
|
I'll try this again. |
But ALSO, noImplicitAny: false seems to change tsc 5.6.3 into inferring "never[]"... Or I am losing my mind. |
No description provided.