-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Add v1 of typescript definition file #9369
Conversation
If complete, this fixes #7909. I'm wondering how the maintenance of this will go. Should we maintain it manually on can we generate this file somehow? |
let's place it in e.g. "external/types/" folder and also split into several files, e.g. display.ts/core.ts/shared.ts |
I manually created this file, and I recommend that it be manually updated. There are many cases where there's a stylistic choice on how to specify the types, and in this code base in particular I think it serves as useful documentation, even for developers who are not using TypeScript. The best way to help maintain it is probably to create a test file that exercises some of the most common APIs. When the APIs change, the test will fail and prompt the developer who changed the code to update the TypeScript definition and tests. I have taken a crack at this with an external test file (not included in this pull request), but I'm having trouble setting up the test scaffolding. Some issues:
|
@yurydelendik It is possible to split the types into separate files, but it introduces several issues:
I think the best solution would be to convert PDFJS itself to TypeScript, then the types would be part of the source files and no parallel type definitions or files would be needed. But this is probably too ambitious :) However I'll say that this type definition file goes a long way towards that goal, if it's something the team would consider. For now I'd suggest we put the effort into adding tests, then we can consider whether refactoring to split the types makes sense. Does that sound ok? |
I'll also add that as I mentioned in the original PR, there's much culling that could probably be done to make the type file smaller. Doing that first might result in a small enough file that there's less of a feeling that it needs to be split up. |
I suppose that my general question/objection here is if people using TypeScript will actually help maintain these definitions, since it seems somewhat unfair to put that burden on every PDF.js contributor considering that the library is only using regular JavaScript!?
In general, the public interface is the one that's found in https://github.com/mozilla/pdf.js/blob/master/src/display/api.js (and perhaps in some of the other files in https://github.com/mozilla/pdf.js/tree/master/src/display); i.e. what ends up in the However, note that all code found in https://github.com/mozilla/pdf.js/tree/master/src/core is running in a web worker, and is thus not even accessible much less part of any public interface. |
We already tracking changes to public API. So modifying/syncing with .d.ts might not be an issue
When TS builds .ts, it generates .d.ts file that references other files. So it's pretty common practice.
As above, IDE will support stuff generated by standard TS compiler.
There is also alternative: Flow. I fonder how easy to convert TS to Flow (or other way around). |
We're still in the process of slowly transitioning the code-base to ES6, and generally more modern JavaScript usage. However, I'd seriously question if it's worth the time and effort required to migrate everything to TypeScript (especially considering the size of the code-base and the number of existing contributors). |
@Snuffleupagus I understand that the intended API is in api.js, but the actual interface exposed includes other types that are required to support those in api.js. Those types in turn expose access to other types. I took the closure of accessible types in this manner to create the type definition file. This is the set of files the types actually come from:
@yurydelendik This is another thing to consider, do we really want to add a To your other question, unfortunately Flow types and TypeScript types aren't compatible at the moment, though it would be a very nice feature to have. My understanding is that TS is getting more traction, and in practice my understanding is that there are more requests for TS than Flow types for pdf.js from the user community. |
If you'd like to see which types come from which files, they're documented in the source code: https://github.com/acchou/pdf.js/blob/41205e3a8dd09e49f710760658a0c9d2168b0381/index.d.ts The regex |
Please keep in mind here that what's returned through the API is not at all the same types that exist in the worker. When data is passed from a web worker to the main thread (or vice versa), via What this means in practice is that if you have e.g. an object on the worker side which is The above is just one very simply example out of many more, but as you can see the types used in the worker doesn't really make sense on the main/API side of things. What's returned by various API methods could probably be thought of more like a proxy to the underlying worker data instead. |
Ok, this was something that I didn't understand. When the types are corrected, they should serve as helpful documentation of what the public API actually looks like. I'll take some time to make this correction; what would be most helpful is to understand how to create a test for this, then I can write actual tests for these types. I need help with the test scaffolding/setup. Where should I put tests for types, and how should I set it up? |
It should be easy to convert: just change classes to interfaces, and remove methods. This is for all data objects returned by For example, I could modify
|
Let's consider
In general it looks like in the pdfjs API, things returned as Currently I have
There are a few things I could change:
Therefore the final type for
Does that sound correct? |
Not quite, since the only thing that's returned for annotations is the Lines 310 to 319 in 96c573a
Hence the A piece of general advice: I'd suggest playing with the API locally to check what's actually returned from the various methods, since it might perhaps be difficult to reason about that theoretically if you're not already familiar with a particular part of the code-base. |
I'll have to do that some more. It's often not enough though, because dynamically it's hard to know if you're seeing everything - there are plenty of optional keys in various types that don't always show up every time, for example. I'll try adding a unit test, that should help document things and also provide dynamic verification of the types. |
Question: |
In |
The |
Ok thanks. Another issue: |
Is
|
I just pushed several changes:
For the viewer components, I exported them by default into the |
Also, the tests are not run as part of any other gulp task, they are separate at the moment. |
I have not done an in-depth review, but I did notice two things. Most of the files are just type definitions, but why does I notice that there may be some unanswered questions above, but maybe in the meantime you already found some answers. If not, could you compile a list of remaining questions in one comment so it's easier for us to answer them? For reviewing, it would help to rebase onto the current master and to squash the commits (see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits if you're not familiar with it). Now there are 21 commits and 24 changes files, which makes it hard to get a good overview of the changes since some changes are just there because of merge commits. After rebasing and squashing, there should be no more merge commits and one (or perhaps two if you want to split the testing from the actual type definitions) functional commits. Thank you! |
For Gulp, I think the best place is Line 1469 in a7cb560
externaltest target already runs some separate test files for code in the external folder, so you can add your tests there too.
|
For the external test, my test requires an The only remaining question I have is how to structure the viewer types. These are currently collected in On the topic of removing I'll squash the commits shortly. |
Squashed. |
Regarding |
After looking at |
Note that the declaration for the I did play around with the TypeScript methods of declaring UMD modules but couldn't figure it out. Maybe someone else who's more familiar with it can issue a future PR if that's an important use case for them. In the meantime, the viewer is accessible as a module. |
Does anyone know the status? |
It's largely complete (as of 3/27 - I haven't checked more recent updates), but the lack of feedback from maintainers means it's basically in limbo... Maybe someone from the team could select one of these options:
I might submit this to |
@yurydelendik Could you chime in for the above? |
@acchou First of all, sorry for the delay here. We put a lot of effort in getting version 2.0 done for release and spare time was a bit limited for me personally. I looked at this again and even though I think the type definitions make sense I still have concerns regarding maintenance, mainly because PDF.js itself does not use any TypeScript and keeping this in sync with the code will be challenging. Therefore, to answer your question above, I personally think it's better to place it in |
Sorry for the delay, Yury and I haven't had much time to work on pdf.js lately. We discussed this a bit recently though and we'd like to accept this and then work on removing duplicate comments and letting the typescript definitions be the documentation for the public API. I think we can accept as is, then do the cleanup. |
Sounds good. Let me know if I can help with any cleanup here. |
Sorry to be that guy.. but any update on the merge for this? The current @types are completely wrong and it makes working with this lib very hard :/ |
I'm also fine with merging this after the 2.0 stable release, which should happen soon, if it's updated as we have added a few more annotation types in the meantime. Since this doesn't touch any core files, I agree with @brendandahl that even more cleanup can be done at a later stage. |
If you can point me in the general direction of where these changes are located, I could update the type annotations. |
annotationType: AnnotationType.POLYLINE; | ||
vertices: Vertex[]; | ||
} | ||
interface PolygonAnnotation extends Annotation { |
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.
The annotation layer changes are in https://github.com/mozilla/pdf.js/blob/master/src/core/annotation.js (core) and https://github.com/mozilla/pdf.js/blob/master/src/display/annotation_layer.js (display). I think InkAnnotation
is missing, but maybe there is more.
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.
Done, see b74c5fd.
… reflect change from Uint8Array to Uint8ClampedArray.
Hi, sorry to be that guy again but, any update on this? @timvandermeij @acchou ? |
I'm still a bit on the fence here. Even though this PR looks good, there is an alternative in #10575 that aims to generate the TypeScript files from type annotations in the docstrings. To me this seems much more maintainable since we don't have to keep multiple files in sync. I think the end result would then be a combination of both, where we move the type annotations from this PR into the docstrings. What do you think? I think providing TypeScript definitions is good, but since it isn't our main focus it should really be easy to maintain. |
I was recently made aware of typescripts |
May I recommend just moving forward with even a manually updated typescript definition? This ticket was updated 1y10m ago... This is REALLY needed for external developers because without it the API is very very very opaque. Even just copying @types/nodejs-dist over and running with that would be good because we can manually merge the types in the mean time as they evolve. Sending a 1 line PR is a lot easier than trying to build an auto-generating system. I can't imagine the core API changes that much in 1y10m where we couldn't have updated it manually. |
I wasn't hoping to build an auto generating system. I was thinking we could have all our typescript definition files and then use things like |
If there are no TS experts in PDF.js team I'd suggest to move these typings to e.g. definitely-typed. Otherwise, we will have a hard time maintaining these typings, with not many people willing & able to review the changes when necessary. Also, this would require contributors to know TypeScript whenever any API change is done. |
Agreed, that seems to be the right thing to do at this point. One thing before I do that though... has anyone tried to use the new --declaration flag with --allowJs (https://www.typescriptlang.org/docs/handbook/release-notes/overview.html#--declaration-and---allowjs)? This seems to allow the typescript compiler to natively generate declaration files from jsdoc annotated javascript source. This seems to be the preferred approach for the team from PR #10575. |
@acchou I don't think anyone tried that yet, but it would be good to look into. I think we've come to the conclusion that manually maintaining the TypeScript bindings here is just not feasible and I agree with @wojtekmaj's analysis here. Let's close this PR and focus our efforts either on getting #10575 landed. We have already merged most of the documentation improvements from that patch in separate PRs, so we can either choose to rebase that PR or make a new PR with the solution where TypeScript natively generates the declaration files from the JSDoc comments. In any case, if we are to provide the bindings, they must be generated in some form to avoid a maintenance burden. Thank you! |
This is a WIP typescript definition file (
index.d.ts
). There are several issues to resolve before this is ready:index.d.ts
to the root of the distribution directorybuild/dist
. If the file is namedindex.d.ts
there is no need for further configuration, it is optional but suggested by the TypeScript docs to add a reference topackage.json
.