Increase type strictness to prepare for Typescript 6 - #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).
…ModuleInterop setting in tsconfig
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress. See below for one new comment based on a re-review of changed files. Also note that the PR is reporting conflicts in package.json, so there's definitely a need to do a merge of upstream/dev -- if it's giving you trouble, let me know and I'll help!
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress here, @Jason-Benson! There's still some rearrangement needed in Config.ts -- see below for lots of comments on that subject, plus one other unrelated thing.
I haven't done any hands-on testing yet, and there are still a couple more files I need to look at, but I think we're getting pretty close to the end of this if nothing is actually broken. :-)
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress, @Jason-Benson. Looks like you've made good progress, but there are still a few unresolved comments. Do you need help with anything, or still just chipping away?
| @@ -0,0 +1,15 @@ | |||
| // Work around for a flaw in @types/pdfkit, which omits `openImage()` from the type definitions. | |||
There was a problem hiding this comment.
Thanks, this is definitely an easier-to-understand version of the fix, and a reasonable solution while we wait for an upstream fix.
If we're impatient for an upstream fix, I wonder if we should try submitting a PR against https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/pdfkit/index.d.ts -- it appears that this repository is fairly active and people have been pushing changes to the pdfkit definitions relatively recently, so more fixes may be accepted.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Jason-Benson, I gave this another thorough review and looked at all of the remaining files. Everything looks good except some more nitpicky type things in the config class. See below for a bunch of suggestions, but please note that none of them are tested, so it's possible I have made mistakes. Please double-check my work! Also note that I spotted one issue in existing code and made note of it in a comment, so watch for that one. :-)
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for all the progress, @Jason-Benson. It looks like two of my last round of comments have not been resolved yet, and that may be because my suggestions were bad (one of them DEFINITELY was), so I'm trying again to make better suggestions this time. ;-)
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Three more little things!
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
One last little bit of cleanup...
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
demiankatz
changed the title
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Everything is looking good and working for me now. Thanks, @Jason-Benson!
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.