fix(types): update to TS 5.3.3 and fix types #2419
Conversation
|
Hi, thanks for your contribution! |
|
@saghul Ready for review! Had to fix a weird issue with the build, which requires overriding the rollup command vitejs/vite#15167 You can see it in the package.json, sorry for such a big PR, but its the only way I could get it to successfully be used in a TS project, as some of the types needed updating |
|
Impressive work, thank you! Quick question before I dive deeper: can't we do something to avoid those weird inline imports in the JSdocs? |
Not that I could find :( It seems that something changed between TypeScript 4 and 5, and referencing global modules. In an ideal world, when this is fully TypeScript, it wont matter, but its because of the way declaration is generated, it needs to reference the appropriate module. |
|
Sorry but that adds too much curn, it's not a change I'm comfortable adding. |
|
Fair enough. I saw your PR, want me to update this one? ill update it to remove the import statements, but keep the fixed types, then move it to the types folder |
|
@saghul I've just pushed an update, it removes the import statements, but still fixes the types and keeps it in the types folder. Fingers crossed it will be easier for TypeScript modules to be used in JSDoc Ive kept TS 5.3.3, as it still fixes the constructor being generated incorrectly. |
|
Thank you! I'll take another look! |
| ], | ||
| "overrides": { | ||
| "rollup": "npm:@rollup/wasm-node" |
There was a problem hiding this comment.
Can you elaborate on why this is necessary?
There was a problem hiding this comment.
Its a weird bug in npm, see my first comment. But its to do with the test framework using rollup, if you look at the logs of one of the early failed builds, it complains it cant find the native rollup.
The fix is to add an override to package.json, as seen here: vitejs/vite#15167
There was a problem hiding this comment.
Specifically this run: https://github.com/jitsi/lib-jitsi-meet/actions/runs/7251356239/job/19753537267
npm bug for reference: npm/cli#4828
…ib-jitsi-meet into dev/fix-uninferred-types
|
Unfortunately, I tried removing the roll-up hack (Blegh 🤢) hoping that the npm issue had been fixed, but it hasnt. I have re-added just so this builds successfully. |
|
Jenkins please test this please. |
|
Let's see if this passes Jenkins. I wonder if there is a problem with Jitsi meet using an older TS... Hopefully not. |
|
After Jenkins passes, can you pl add a new commit basically undoing this: ca40744 so we can test it that way too? |
|
Passed on Jenkins, Added the type line back back |
|
Jenkins please test this please. |
Took a bit longer than expected, as there were a lot of types to update.