Typescript6 update#367
Conversation
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Jason-Benson! I didn't have time to review this very thoroughly, but I have a few questions and comments below that might be helpful.
There is already quite a lot going on in this PR -- I'd suggest if you do any more, start separate PRs so we can discuss separate issues in separate places, and get things merged as they are finished. Otherwise, this will become hard to keep track of! (I realize it's a balancing act, because if you split things up into too many separate branches, that can lead to merge conflicts and other problems... but I think in this case there's a lot of potential for working in smaller chunks without running into that kind of trouble).
|
|
||
| get models(): Record<string, FedoraModel> { | ||
| return this.ini["models"] || {}; | ||
| return (this.ini["models"] as unknown as Record<string, FedoraModel>) || {}; |
There was a problem hiding this comment.
Why do we need the as unknown here but not in other places?
There was a problem hiding this comment.
It appears that we do not.
|
|
||
| get agentDefaults(): Record<string, string> { | ||
| return this.ini?.["agent"]?.["defaults"] ?? {}; | ||
| return ((this.ini["agent"] as ConfigRecord)?.["defaults"] as Record<string, string>) ?? {}; |
There was a problem hiding this comment.
In cases like this, do we need to account for the possibility of an undefined value?
| return ((this.ini["agent"] as ConfigRecord)?.["defaults"] as Record<string, string>) ?? {}; | |
| return ((this.ini["agent"] as ConfigRecord | undefined)?.["defaults"] as Record<string, string>) ?? {}; |
There was a problem hiding this comment.
I honestly don't know if that's needed, but it works so I'll follow your suggestion.
| "supertest": "^7", | ||
| "ts-jest": "^29", | ||
| "typescript": "^5", | ||
| "typescript": "^6", |
There was a problem hiding this comment.
Did you mean to upgrade typescript here? I thought we were going to try to improve types using typescript 5 before actually upgrading anything.
There was a problem hiding this comment.
I had forgotten about that. I backed it off to 5 and got the same test results, so I'll leave it on 5 till the currently broken tests are resolved.
| @@ -1,10 +1,7 @@ | |||
| import sharp from "sharp"; | |||
There was a problem hiding this comment.
I'm not sure why these sharp changes are showing up here -- they have already been merged to dev. Maybe you need to merge upstream/dev into this branch to straighten things out.
There was a problem hiding this comment.
I tried it, but it just reported that I was already up to date.
There was a problem hiding this comment.
Did you git fetch upstream before the merge? I can dig into it more deeply tomorrow if it's not that simple. :-)
2 participants
This is the first in a set of updates to allow typescript 6.
One notable change from TS5 is that strict is now implicit, and I updated tsconfig to match previous behavior. A lot more work is needed before ts6 can be used.