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

Improve type definitions for the viewer #17879

Merged
merged 1 commit into from
Apr 14, 2024
Merged

Conversation

ex37
Copy link
Contributor

@ex37 ex37 commented Apr 3, 2024

This commit improves compatibility of the viewer code with TypeScript by including missing type imports/definitions and correcting existing ones

@Snuffleupagus
Copy link
Collaborator

Given that the default viewer is not intended to be directly used by third-parties, and thus isn't included in pdfjs-dist, I don't know if this is really desirable. Please see http://mozilla.github.io/pdf.js/getting_started/#introduction (emphasis mine):

The viewer is built on the display layer and is the UI for PDF viewer in Firefox and the other browser extensions within the project. It can be a good starting point for building your own viewer. However, we do ask if you plan to embed the viewer in your own site, that it not just be an unmodified version. Please re-skin it or build upon it.

@ex37
Copy link
Contributor Author

ex37 commented Apr 3, 2024

@Snuffleupagus this could help people who want to extend the viewer. Plus there are also lots of nice utils within the web folder. Also added jsdoc types could be helpful for project's contributors.

But if you think this is not within the scope of the project, we can remove the entrypoint, and just add the type fixes.

@Snuffleupagus
Copy link
Collaborator

But if you think this is not within the scope of the project, we can remove the entrypoint, and just add the type fixes.

That'd be a requirement as far as I'm concerned. Please note that the existing type definitions are unused within the PDF.js project, but has nonetheless added a bunch of work for the core contributors; hence we don't want to further increase that.

Also, please make sure to write a good commit message.

web/annotation_editor_params.js Show resolved Hide resolved
web/base_tree_viewer.js Show resolved Hide resolved
web/base_tree_viewer.js Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@ex37 ex37 changed the title generate typings for web app improve viewer typings Apr 3, 2024
Copy link
Contributor Author

@ex37 ex37 left a comment

Choose a reason for hiding this comment

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

Addressed requested changes, changed commit message / pr name.
Also added eslint-disable-next-line where needed.

@ex37 ex37 requested a review from Snuffleupagus April 9, 2024 13:09
web/annotation_editor_params.js Outdated Show resolved Hide resolved
web/annotation_editor_params.js Outdated Show resolved Hide resolved
web/external_services.js Outdated Show resolved Hide resolved
web/external_services.js Outdated Show resolved Hide resolved
web/pdf_attachment_viewer.js Outdated Show resolved Hide resolved
web/pdf_layer_viewer.js Outdated Show resolved Hide resolved
web/pdf_outline_viewer.js Outdated Show resolved Hide resolved
@ex37 ex37 requested a review from Snuffleupagus April 10, 2024 10:40
@Snuffleupagus
Copy link
Collaborator

@ex37
Copy link
Contributor Author

ex37 commented Apr 10, 2024

squashed

web/grab_to_pan.js Outdated Show resolved Hide resolved
@timvandermeij
Copy link
Contributor

timvandermeij commented Apr 12, 2024

Before we can merge this please also include a better commit message. The guidelines at https://cbea.ms/git-commit/ could help with this. The commit message should primarily contain which change is being made and why; implementation details like which files or classes are changed can usually be left out because that's already clear from the commit diff.

The following commit message (or something similar) could be a suggestion here:

Improve type definitions for the viewer

This commit improves compatibility of the viewer code with TypeScript by
including missing type imports/definitions and correcting existing ones.

@ex37
Copy link
Contributor Author

ex37 commented Apr 12, 2024

Got it. Set to your suggested description, as it fully describes what this pr is about.

@timvandermeij timvandermeij changed the title improve viewer typings Improve type definitions for the viewer Apr 12, 2024
Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

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

Looks good to me with the comments below addressed, but given the original review I'll leave the final approval to @Snuffleupagus. Thanks!

web/base_tree_viewer.js Outdated Show resolved Hide resolved
web/pdf_document_properties.js Outdated Show resolved Hide resolved
web/pdf_outline_viewer.js Outdated Show resolved Hide resolved
This commit improves compatibility of the viewer code with TypeScript by including missing type imports/definitions and correcting existing ones
@ex37
Copy link
Contributor Author

ex37 commented Apr 12, 2024

removed the default value for hidden, and also added missing extensions

@ex37
Copy link
Contributor Author

ex37 commented Apr 12, 2024

@timvandermeij in my opinion a proper fix for _addToggleButton method would probably be separating it into two methods, which will be using same signature across inheriting classes. Other than type definitions, in my understanding, it can also help with runtime optimizations. But yes, that's outside of this pr scope.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

r=me, thanks.

@timvandermeij timvandermeij merged commit e08de77 into mozilla:master Apr 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants