mozilla / pdf.js

PDF Reader in JavaScript
https://mozilla.github.io/pdf.js/
Apache License 2.0
48.04k stars 9.93k forks source link

Update `typescript` to version 5.6.2 #18770

Open timvandermeij opened 2 days ago

timvandermeij commented 2 days ago

I have tried to perform this update, but the types test fail when doing so.

Steps to reproduce:

  1. Update the typescript dependency to version 5.6.2 in package.json and install it.
  2. Run npx gulp typestest.
  3. Notice that it fails with the following error: Couldn't compile TypeScript test: build/typestest/types/src/display/display_utils.d.ts(89,19): error TS2339: Property 'docId' does not exist on type '{ ownerDocument?: Document | undefined; }'.

The error seems to happen on line https://github.com/mozilla/pdf.js/blob/master/src/display/display_utils.js#L66, but AFAICT this is valid code.

/cc @tamuratak @ineiti Could one of you perhaps help out with this?

tamuratak commented 19 hours ago

The most simple workaround would be adding an any annotation:

diff --git a/src/display/display_utils.js b/src/display/display_utils.js
index f88dd3041..d6d9c62a8 100644
--- a/src/display/display_utils.js
+++ b/src/display/display_utils.js
@@ -63,6 +63,9 @@ class DOMFilterFactory extends BaseFilterFactory {

   #id = 0;

+  /**
+   * @param {any} [param0]
+   */
   constructor({ docId, ownerDocument = globalThis.document } = {}) {
     super();
     this.#docId = docId;

An appropriate patch might be:

diff --git a/src/display/display_utils.js b/src/display/display_utils.js
index f88dd3041..224379738 100644
--- a/src/display/display_utils.js
+++ b/src/display/display_utils.js
@@ -63,6 +63,9 @@ class DOMFilterFactory extends BaseFilterFactory {

   #id = 0;

+  /**
+   * @param {{ docId: number | undefined, ownerDocument: Document | undefined}} [param0]
+   */
   constructor({ docId, ownerDocument = globalThis.document } = {}) {
     super();
     this.#docId = docId;
ineiti commented 12 hours ago

That looks great. Both me and ChatGPT think the second could be simplified into the following. However I'm not sure if this fits in with the other annotations, and/or if both me and ChatGPT hallucinate things which are not possible :)

diff --git a/src/display/display_utils.js b/src/display/display_utils.js
index f88dd3041..224379738 100644
--- a/src/display/display_utils.js
+++ b/src/display/display_utils.js
@@ -63,6 +63,9 @@ class DOMFilterFactory extends BaseFilterFactory {

   #id = 0;

+  /**
+   * @param {{ docId?: number, ownerDocument?: Document}} [param0]
+   */
   constructor({ docId, ownerDocument = globalThis.document } = {}) {
     super();
     this.#docId = docId;
timvandermeij commented 51 minutes ago

Thank you both for the assistance! While trying this out I noticed that I couldn't reproduce the issue anymore, and found that commit bb302dd99337ed4705f2b5d4000091d844366b78 which landed a few hours ago (accidentally) fixed the issue. TypeScript apparently got confused about the default value we provided there before, but now that we no longer do that the tests are green again, so fortunately we can simply proceed with the update now without changes to the code. I have created PR https://github.com/mozilla/pdf.js/pull/18781 to update the dependency.