Generate .d.ts files from jsdoc information #10575

oBusk · 2019-02-22T23:44:42Z

I was looking to update https://www.npmjs.com/package/@types/pdfjs-dist to 2.0.0 but figured it would be better if the package contained the typings and tried to use https://www.npmjs.com/package/tsd-jsdoc to generate them.

This is a start for generating .d.ts files from JSDoc to provide typeinformation for developers.

I have no idea if the generated types.d.ts is usable right now.

Will also need some additions to gulpfile to add types to package.json and ship the typefile. Not sure if I can get that together.

Related: #9369, #7909

Snuffleupagus · 2019-02-23T00:30:57Z

gulpfile.js

@@ -948,7 +949,10 @@ gulp.task('jsdoc', function (done) {

  var JSDOC_FILES = [
    'src/doc_helper.js',
+    'src/interfaces.js',
+    'src/core/primitives.js',


This file makes absolutely no sense to include in API documentation, since it contains functionality which only runs in a web worker and isn't even accessible for an API consumer anyway (the same goes for most/all code in the src/core/ folder); please see #9369 (comment) for additional information.

Also, what's the point of this PR when there's already the #9369 in progress, which seems much more complete overall?

Ok I see. Would it then make more sense to declare the interfaces that are used in the API with JSDoc so that the documentation can refer to them without risking confusing them for the datatypes used internally in the worker?

I suppose I'd try to mock them up in doc_helper so that they may be corrected later.

Generating .d.ts files from JSDoc means less maintance because you don't have to remember to edit the external .d.ts file. Another bonus is that maintaining the JSDoc comments for dts typings would improve/structure the API documentation and vice versa. I'm not guaranteeing that it'd be perfect, but to me it seems like it would be useful.

timvandermeij · 2019-02-23T14:21:07Z

Even though this doesn't seem complete yet, I do like the idea of generating the ts files from the JSDoc comments. For the other open PR, I'm still a bit unsure about maintenance since keeping multiple files in sync can be error-prone. This would allow us to still provide ts files and don't have the problem of added maintenance.

Perhaps you can turn this PR into a working prototype that contains the code to generate those files during a build and to fix up one or two classes with the required type annotations in the JSDocs? That way we can check how useful the output is without putting in a lot of work in this PR to convert everything at once. This also allows us to merge the ground work and extend it in chunks later on.

oBusk · 2019-02-25T22:09:21Z

I've updated gulpfile.js to include the types when building, and with the addition to declare api.js as the module pdfjs-dist, I managed to import and get basic typing. So I guess the following steps would be to remove the internal files that I incorrectly included to get the types like Name etc., from the generating and declaring the necessary data types in either api.js or doc_helper.js.

Was that a correct interpretation of your previous comment, @timvandermeij?

timvandermeij · 2019-02-25T22:53:07Z

Sounds good to me. It's mainly about making a minimal working version so it's easier to review and we can iterate on it.

oBusk · 2019-02-27T01:27:09Z

@timvandermeij I took another swing, focusng on the files that was inputed into the JSDoc API documentation script originally. It's outputting a error free typingsfile and the basic typings seems to be correct.

timvandermeij

Looks much better this way! I'm confident in the approach with regards to generating the typing files from the JSDoc comments since it makes maintenance and correctness much easier in my opinion. I left some comments after looking at this. I'm also curious what other contributors/maintainers think about this approach.

timvandermeij · 2019-02-27T22:25:16Z

src/core/annotation.js

@@ -494,6 +496,11 @@ class AnnotationBorderStyle {
    }
  }

+  /**


Since this is only for documentation/typing purposes, could we perhaps find a way to move these kind of generic typings to a central place, for example https://github.com/mozilla/pdf.js/blob/master/src/doc_helper.js? The same applies to other of these blocks below.

timvandermeij · 2019-02-27T22:27:21Z

src/display/api.js

@@ -883,42 +919,49 @@ class PDFPageProxy {
  }

  /**
-   * @return {number} Page number of the page. First page is 1.
+   * @type {number} Page number of the page. First page is 1.


In the other files we still have return, so why is that changed to type here and below? It actually represents the return value of the function.

The .d.ts was generated with no types when using @return, and as far as I could figure out, get properties should be documented like members using @type. In the current html API (draft) documentation you can see that the type is not shown https://mozilla.github.io/pdf.js/api/draft/PDFPageProxy.html#pageNumber

Here is a comparison

Using @return

Using @type

The reason why I did not change in all files was that i was focusing on api.js.

Good point, I had no idea that there was actually a difference here. It's probably best to keep it as-is for this patch and create a follow-up issue to adjust the other places later.

acchou · 2019-12-27T03:11:01Z

TypeScript 3.7 has native support for generating declaration files from jsdoc annotated source: https://www.typescriptlang.org/docs/handbook/release-notes/overview.html#--declaration-and---allowjs

Seems worth a try.

timvandermeij · 2019-12-27T11:24:07Z

Most of the documentation improvements from this PR have already been merged in other PRs, so I would suggest to rebase this first to see which changes are left, since we concluded that the approach of generating the TypeScript bindings from JSDoc comments is the preferred approach. We can then choose to keep this way of generating the bindings, or look into the way from #10575 (comment) which also looks promising.

oBusk · 2019-12-27T17:50:00Z

@timvandermeij That sounds like a sensible way to approach the matter. I'll rebase and see how that turns out. Then I'll try typescript 3.7 like @acchou suggested.

Multiple places refer to the class PDFDocumentLoadingTask, which is defined inside a closure. By using `@lends <global>` we make the class global and thus will be included in the typescript output. https://stackoverflow.com/a/22324161/1433417 https://jsdoc.app/about-namepaths.html https://jsdoc.app/tags-lends.html

oBusk · 2019-12-28T13:41:47Z

@timvandermeij I managed to get it to work again.

Types look something like this right now;
https://gist.github.com/oBusk/8bdbbdae7d5b23fd419c4af3fff8f57c

Snuffleupagus · 2019-12-28T13:58:45Z

src/display/api.js

@@ -407,6 +420,7 @@ function _fetchDocument(worker, source, pdfDataRangeTransport, docId) {
    });
 }

+/** @lends <global> */


We're already exposing a bunch interfaces for the unit-tests, hence please try to see if simply listing PDFDocumentLoadingTask with the exports at the bottom of the file works instead.

Exporting a const that happens to be assigned to a class does not mean that class get's exported. And I don't know if there's any way to export the class itself from within the closure.

Snuffleupagus · 2019-12-28T14:00:42Z

src/display/api.js

- * @property {Array} fnArray - Array containing the operator functions.
- * @property {Array} argsArray - Array containing the arguments of the
- *                               functions.
+ * @property {Array<Function>} fnArray - Array containing the operator


This looks strange, since there's nothing which is typeof "function" in the array.

You're right. I think I just guessed based on the name. I ran an example and set the type to be Array<number> instead.

Snuffleupagus · 2019-12-28T14:02:47Z

As mentioned previously, please remove any src/core/ changes (or references to such files) since that code runs in a worker-thread and is thus not actually accessible directly in the API.

Furthermore, as also mentioned previously, please place any typescript specific definitions in a helper file (e.g. src/doc_helper.js) rather than directly in the API and let's avoid a bunch of TODO comments in this code.

Finally, please note https://github.com/mozilla/pdf.js/wiki/Squashing-Commits

timvandermeij · 2019-12-28T15:44:36Z

gulpfile.js

+  console.log();
+  console.log("### Generating typescript definitions");
+
+  var JSDOC_FILES = [


This changed in the jsdoc target since this patch was made I think, so making this equal to https://github.com/mozilla/pdf.js/pull/10575/files#diff-b9e12334e9eafd8341a6107dd98510c9R1092 allows you to remove all src/core and src/shared changes from this patch and include all TypeScript-specific constructs in src/doc_helper.js as indicated by @Snuffleupagus.

timvandermeij · 2019-12-28T15:49:07Z

src/display/api.js

@@ -82,13 +87,19 @@ function setPDFNetworkStreamFactory(pdfNetworkStreamFactory) {
  createPDFNetworkStream = pdfNetworkStreamFactory;
 }

+/* eslint-disable max-len */
+/**
+ * @typedef {Int8Array | Uint8Array | Int16Array | Uint16Array | Int32Array | Uint32Array | Uint8ClampedArray | Float32Array | Float64Array} TypedArray //


We should be able to remove the trailing // here.

timvandermeij · 2019-12-28T15:58:17Z

The code looks simpler already and the generated types also look good, so it's good to see that this approach is working well. We have left a few comments above to simplify the code even further.

oBusk · 2019-12-28T17:23:11Z

I've the comments and I'll try to look at them when I have time

I just felt like I should share what I found; I feel like using typescript 3.7 is probably a more "correct" way forward, as tsd-jsdoc is far less used than typescript (obviously). I just quickly tried but ran into similiar issues of refering to classes that are not declared globally and thus not able to export them, meaning tsc will fail with

src/display/api.js:21:1 - error TS9005: Declaration emit for this file requires using private name 'PDFDocumentLoadingTask'. An explicit type annotation may unblock declaration emit.

I can't figure out a workaround.

It was previously specified as `function` probably based on a guess by the name. The array itself seems to always be numbers.

P0oOOOo0YA · 2020-02-11T20:26:01Z

OK so what's the best approach here for Typescript developers? Install typescript 3.7 and let the typescript parser go through the source, parse JSDoc and build the types dynamically? At least there should be some hints for TS developers on the README.

oBusk · 2020-02-12T19:25:49Z

@P0oOOOo0YA if you want to use pdfjs in your typescript project, currently your option is to install and use https://www.npmjs.com/package/@types/pdfjs-dist

I've tried somewhat to create typings from the source, but using typescript I ran into issues due to the structure of the JS code that I don't grasp enough to really figure out what to do with typescript.

tamuratak · 2020-02-13T09:19:43Z

I have made a PR, oBusk/pull/1, to make tsc generates declaration files of the PDF.js repository. We have to add type annotations to some exported constants explicitly.

buzinas · 2020-07-09T21:36:23Z

@oBusk Can you look into @tamuratak's PR above?

ineiti · 2020-07-18T22:50:56Z

I tried to merge @tamuratak's PR and fix the conflicts here: #12102

For compiling, I have to do both the gulp generic and npx tsc -p ., else I get a mostly empty worker.js...

Unfortunately I cannot get it to run: https://github.com/ineiti/pdf_example doesn't seem to find the correct worker. It fails with:

UnknownErrorException: The API version "null" does not match the Worker version "2.4.456".

Even though it 'should' be OK: https://github.com/ineiti/pdf_example/blob/master/src/app/home/home.component.ts

BTW: I'm re-routing the paths using tsconfig: https://github.com/ineiti/pdf_example/blob/master/src/tsconfig.app.json

And there is some work still to be done to have cleaner typescript definitions, because quite a lot of the definitions return an Object, which is not ideal to work with...

timvandermeij · 2020-07-19T12:51:07Z

Let's continue this discussion in the new PR since it's more up-to-date than this one.

Snuffleupagus reviewed Feb 23, 2019

View reviewed changes

timvandermeij added the other label Feb 23, 2019

oBusk force-pushed the add-dts branch from c33e65d to 4b438ad Compare February 27, 2019 01:17

timvandermeij reviewed Feb 27, 2019

View reviewed changes

timvandermeij mentioned this pull request May 20, 2019

Import pdf.js with typescript #10547

Closed

timvandermeij mentioned this pull request Sep 20, 2019

Add v1 of typescript definition file #9369

Closed

4 tasks

This was referenced Oct 6, 2019

Use square brackets for optional properties in the JSDoc comments of src/display/api.js #11209

Merged

Delete the @ignore tag in the JSDoc comment for the PDFObjects class #11210

Closed

burtonator mentioned this pull request Oct 13, 2019

pdfjs bindings for upgraded 2.2.228 version. Existing versions are stale. I don't DefinitelyTyped/DefinitelyTyped#39096

Closed

9 tasks

oBusk added 4 commits December 28, 2019 01:07

Add script for building d.ts files from JSDoc and include in dist

608cbe7

Fix JSDoc enough to generate error-free typings

3ed3b0d

Add @module to make import pdfjs-dist work

8d01b5d

Fix classes

d52e145

oBusk force-pushed the add-dts branch from 4b438ad to d52e145 Compare December 28, 2019 09:13

oBusk added 3 commits December 28, 2019 10:21

Fix double blank lines

3230511

Run prettier on gulpfile.js

1abb8a9

Snuffleupagus reviewed Dec 28, 2019

View reviewed changes

timvandermeij reviewed Dec 28, 2019

View reviewed changes

oBusk added 2 commits December 29, 2019 11:21

Merge branch 'master' of github.com:mozilla/pdf.js into add-dts

6edf0f2

Fix type for PDFOperatorList.fnArray

3941bb8

It was previously specified as `function` probably based on a guess by the name. The array itself seems to always be numbers.

timvandermeij mentioned this pull request Jan 19, 2020

Feature request: Add type definitions for Typescript and make it available on NPM. #7909

Closed

ineiti mentioned this pull request Jul 18, 2020

Add types annotations #12102

Merged

timvandermeij closed this Jul 19, 2020

oBusk deleted the add-dts branch July 23, 2020 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate .d.ts files from jsdoc information #10575

Generate .d.ts files from jsdoc information #10575

oBusk commented Feb 22, 2019 •

edited

Snuffleupagus Feb 23, 2019

oBusk Feb 25, 2019

timvandermeij commented Feb 23, 2019 •

edited

oBusk commented Feb 25, 2019 •

edited

timvandermeij commented Feb 25, 2019 •

edited

oBusk commented Feb 27, 2019

timvandermeij left a comment

timvandermeij Feb 27, 2019

timvandermeij Feb 27, 2019

oBusk Feb 28, 2019

timvandermeij Feb 28, 2019

acchou commented Dec 27, 2019

timvandermeij commented Dec 27, 2019

oBusk commented Dec 27, 2019

oBusk commented Dec 28, 2019

Snuffleupagus Dec 28, 2019

oBusk Dec 29, 2019

Snuffleupagus Dec 28, 2019

oBusk Dec 30, 2019

Snuffleupagus commented Dec 28, 2019

timvandermeij Dec 28, 2019

timvandermeij Dec 28, 2019

timvandermeij commented Dec 28, 2019

oBusk commented Dec 28, 2019

P0oOOOo0YA commented Feb 11, 2020 •

edited

oBusk commented Feb 12, 2020

tamuratak commented Feb 13, 2020

buzinas commented Jul 9, 2020

ineiti commented Jul 18, 2020 •

edited

timvandermeij commented Jul 19, 2020

@@ @@ -494,6 +496,11 @@ class AnnotationBorderStyle { @@
                   }
                 }
+                /**

Generate .d.ts files from jsdoc information #10575

Generate .d.ts files from jsdoc information #10575

Conversation

oBusk commented Feb 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Feb 23, 2019 • edited

oBusk commented Feb 25, 2019 • edited

timvandermeij commented Feb 25, 2019 • edited

oBusk commented Feb 27, 2019

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acchou commented Dec 27, 2019

timvandermeij commented Dec 27, 2019

oBusk commented Dec 27, 2019

oBusk commented Dec 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Dec 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Dec 28, 2019

oBusk commented Dec 28, 2019

P0oOOOo0YA commented Feb 11, 2020 • edited

oBusk commented Feb 12, 2020

tamuratak commented Feb 13, 2020

buzinas commented Jul 9, 2020

ineiti commented Jul 18, 2020 • edited

timvandermeij commented Jul 19, 2020

oBusk commented Feb 22, 2019 •

edited

timvandermeij commented Feb 23, 2019 •

edited

oBusk commented Feb 25, 2019 •

edited

timvandermeij commented Feb 25, 2019 •

edited

P0oOOOo0YA commented Feb 11, 2020 •

edited

ineiti commented Jul 18, 2020 •

edited