-
Notifications
You must be signed in to change notification settings - Fork 10k
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 Viewer API definitions and include in CI #13930
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand the "add CI check task"-part of the commit message, is there something missing in the patch?
Also, it probably wouldn't hurt to actually mention (in the first line of the commit message) that it's the TypeScript definitions for the viewer that's being changed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with these comments addressed (and some of the remaining ones above). Thank you for improving the types and their testing!
The Viewer API definitions do not compile because of missing imports and anonymous objects are typed as `Object`. These issues were not caught during CI because the test project was not compiling anything from the Viewer API. As an example of the first problem: ``` /** * @implements MyInterface */ export class MyClass { ... } ``` will generate a broken definition that doesn’t import MyInterface: ``` /** * @implements MyInterface */ export class MyClass implements MyInterface { ... } ``` This can be fixed by adding a typedef jsdoc to specify the import: ``` /** @typedef {import("./otherFile").MyInterface} MyInterface */ ``` See jsdoc/jsdoc#1537 and microsoft/TypeScript#22160 for more details. As an example of the second problem: ``` /** * Gets the size of the specified page, converted from PDF units to inches. * @param {Object} An Object containing the properties: {Array} `view`, * {number} `userUnit`, and {number} `rotate`. */ function getPageSizeInches({ view, userUnit, rotate }) { ... } ``` generates the broken definition: ``` function getPageSizeInches({ view, userUnit, rotate }: Object) { ... } ``` The jsdoc should specify the type of each nested property: ``` /** * Gets the size of the specified page, converted from PDF units to inches. * @param {Object} options An object containing the properties: {Array} `view`, * {number} `userUnit`, and {number} `rotate`. * @param {number[]} options.view * @param {number} options.userUnit * @param {number} options.rotate */ ```
@timvandermeij I noticed that this project doesn't squash when merging so I've rebased and squashed to a single commit to keep things clean. |
There are some things defined in
As it is, I have to do a sub-module import, which is fragile if these items get refactored into different files:
Out of curiosity:
|
That's correct; we don't use separate fixup commits in this project.
No, those are not supposed to be directly exported through the API since a user is never intended to create/initialize them manually.
I'd suggest implementing the mentioned work-around in a separate PR, since it's not directly related to this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much better, thank you!
(I'll let Tim take one last look as well, before we merge this.)
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/4717b69adbab00e/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/4717b69adbab00e/output.txt Total script time: 5.20 mins Published |
Thank you for contributing this! |
Thank you both for your patience as reviewers! |
The Viewer API definitions do not compile because of missing imports and
anonymous objects are typed as
Object
. These issues were not caughtduring CI because the test project was not compiling anything from the
Viewer API.
As an example of the first problem:
will generate a broken definition that doesn’t import MyInterface:
This can be fixed by adding a typedef jsdoc to specify the import:
See jsdoc/jsdoc#1537 and
microsoft/TypeScript#22160 for more details.
As an example of the second problem:
generates the broken definition:
The jsdoc should specify the type of each nested property: