Conversation
Coobaha
force-pushed
the
feature/ts5
branch
4 times, most recently
from
2e93b31 to
b2dae31
Compare
There was a problem hiding this comment.
👏 Mostly did a quick check and pushed minor fixes while reviewing 👼
There are some unsound places due to typecasting. Fixing them would require converting many parts to be more static and may not be worth it as it could require significant changes and new bugs.
- I noticed that the dev command logs errors.
-
bumped the CI base image to upgrade the Node.js version to v20 so it works with the new syntax.
-
bumped jest/eslint deps related to TS as they were broken (ts-jest supports ts5.4, and at least in my editor eslint was not working)
-
I think tests and editor are using different
configas I had no errors in the editor,r but tests were showingtscerrors
| fallbackAnimation[key] = | ||
| fallbackTarget === undefined ? null : fallbackTarget | ||
| // @ts-expect-error - @mattgperry to figure if we should do something here | ||
| fallbackAnimation[key] = fallbackTarget ?? null |
There was a problem hiding this comment.
@mattgperry Noticed here a valid ts error, as type of fallbackAnimation[key] is number | string and it assigns null
| expect(ease![0]).toEqual("linear") | ||
| expect(ease![1]).toEqual("linear") | ||
| expect(typeof ease![2]).toEqual("function") | ||
| expect((ease as Easing[])[0]).toEqual("linear") |
There was a problem hiding this comment.
@mattgperry
There is degradation in type-checking.
It’s safer to use non-null assertion than as.
https://www.reddit.com/r/typescript/comments/11rqtn3/is_as_any_better_than_to_assert_nonnullishness/
|
|
||
| for (const key in props) { | ||
| const prop = props[key] | ||
| const prop = props[key as keyof typeof props] |
There was a problem hiding this comment.
@mattgperry how about depending on:
- function’s return type (it handles cases when type-checking of Framer Motion is done outside of this repo, i.e by users-devs)
- and global per-repo type override for loops, so the key is auto-inferred from the array/object (it handles cases when type-checking is performed within the repo)
- you should also consider to switch to
for..of+ Object.keys for a safer and faster loop - https://dev.to/tmhao2005/typescript-override-typing-for-objectkeys-5dmh
- you should also consider to switch to
It would ensure cases like this don’t need manual as override, removing possibilities of human errors.
If you still want to manually define key’s type, then you should do it once before the for loop.
3 participants
Fixes #2592