Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(types): update to TS 5.3.3 and fix types #2419

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

DanielMcAssey
Copy link
Contributor

@DanielMcAssey DanielMcAssey commented Dec 18, 2023

Took a bit longer than expected, as there were a lot of types to update.

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@DanielMcAssey
Copy link
Contributor Author

DanielMcAssey commented Dec 18, 2023

@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

@saghul
Copy link
Member

saghul commented Dec 18, 2023

Impressive work, thank you!

Quick question before I dive deeper: can't we do something to avoid those weird inline imports in the JSdocs?

@DanielMcAssey
Copy link
Contributor Author

DanielMcAssey commented Dec 18, 2023

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.

@saghul
Copy link
Member

saghul commented Dec 19, 2023

Sorry but that adds too much curn, it's not a change I'm comfortable adding.

@DanielMcAssey
Copy link
Contributor Author

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

@DanielMcAssey
Copy link
Contributor Author

DanielMcAssey commented Dec 19, 2023

@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.

@saghul
Copy link
Member

saghul commented Dec 19, 2023

Thank you! I'll take another look!

],
"overrides": {
"rollup": "npm:@rollup/wasm-node"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why this is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@DanielMcAssey DanielMcAssey Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielMcAssey
Copy link
Contributor Author

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.

saghul
saghul previously approved these changes Jan 8, 2024
@saghul
Copy link
Member

saghul commented Jan 8, 2024

Jenkins please test this please.

@saghul
Copy link
Member

saghul commented Jan 8, 2024

Let's see if this passes Jenkins. I wonder if there is a problem with Jitsi meet using an older TS... Hopefully not.

@saghul
Copy link
Member

saghul commented Jan 8, 2024

After Jenkins passes, can you pl add a new commit basically undoing this: ca40744 so we can test it that way too?

@DanielMcAssey
Copy link
Contributor Author

DanielMcAssey commented Jan 8, 2024

Passed on Jenkins, Added the type line back back

@saghul
Copy link
Member

saghul commented Jan 8, 2024

Jenkins please test this please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants