Convert and refactor utility modules to TypeScript#1250
Conversation
Simon1375
commented
Feb 20, 2026
PhilippWendler
left a comment
There was a problem hiding this comment.
Thanks a lot!
The new types are quite nice, their names are good.
The PR basically contains a few different kinds of changes:
- definitions of new types
- added types for parameters etc.
- necessary adjustments to satisfy the type checker, like fallbacks for
undefined, type casts etc. - some cleanup where the old code misused the dynamic typing
- a few cases where local variables seem to be renamed without reason (
filtertofor so)
Except for the 1-2 cases where I left comments I did not see anything where the new code seems to be worse than the old code, mostly it is is a noticeable improvement.
Not sure why there are some renamings that seem to not be motivated by the added types, maybe for future PRs this can be avoided.
Differently from what I had hoped the differences are large enough such that a really thorough review that checks in a detailed manner whether there are behavioral differences would take too much time. This is unfortunate, but I would still continue with the plan and hope that our tests would catch differences. Maybe we can retry a review after finding a way to convert the TS code to similar plain JS code. And at least in the end we should add a round of manual testing of all the functionality.
Some of the added code for satisfying the type checker is ugly, but this does not happen too often. I am optimistic that these cases can eventually be resolved when we continue to improve the code further in general and use types more consistently.
So in total I think this shows that the TS migration (a) works and (b) makes the code significantly better due to the new types and the related improvements. 👍
We can merge this PR after the small cleanups from my comments.
|
Do you want to remove the comments with migration notes before merging this PR or only before merging into main? |
I suggest removing them now. |
3fbe608
into
typescript-migration/typescript-full-migration
|
For the record: It seems this PR broke adding filters via the sidebar. #1253 has a fix, if I understand correctly it is this one: #1253 (comment) |