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

Clean Up theia.d.ts #11493

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Aug 1, 2022

Depends on #11564.

What it does

There are a number of syntactic (misplaced readonly modifiers) and semantic (use of undeclared variables) problems in our theia.d.ts file. This cleans them up.

We can detect these problems by setting skipLibCheck: false in packages/plugin/tsconfig.json, but that makes it impossible to compile the application because we have a lot of conflicting declarations in our node modules.

How to test

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added quality issues related to code and application quality plug-in system issues related to the plug-in system labels Aug 1, 2022
@colin-grant-work colin-grant-work force-pushed the bugfix/theia-d-ts-cleanup branch 3 times, most recently from 229f84a to b5ea5c8 Compare August 11, 2022 16:57
@colin-grant-work colin-grant-work marked this pull request as ready for review August 11, 2022 16:59
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Looks mostly good, thanks for the cleanup!

* enables reusing existing code without migrating to a specific promise implementation. Still,
* we recommend the use of native promises which are available in this editor.
*/
interface Thenable<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this type already defined in @theia/plugin? Is it necessary to re-declare it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to suggestions. It could be declared only in theia.proposed.d.ts and then picked up automatically in theia.d.ts but it's good for it to be in theia.d.ts for the use of the comparator.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes overall look good to me 👍 The only thing I noticed was how our typedocs generation would fail to produce any meaningful content now for the @theia/plugin extension.

packages/plugin/src/theia.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍 It'll definitely help with all the false positives we used to see and would now be correctly identified with the comparator updates.

@colin-grant-work colin-grant-work merged commit 77e8a60 into eclipse-theia:master Aug 30, 2022
@colin-grant-work colin-grant-work deleted the bugfix/theia-d-ts-cleanup branch August 30, 2022 15:14
@colin-grant-work colin-grant-work added this to the 1.30.0 milestone Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system quality issues related to code and application quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants