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

Rename *.d.ts to *.d.mts #17252

Merged
merged 1 commit into from
Nov 12, 2023
Merged

Rename *.d.ts to *.d.mts #17252

merged 1 commit into from
Nov 12, 2023

Conversation

tamuratak
Copy link
Contributor

Rename *.d.ts to *.d.mts. Close #17241

TypeScript doesn't look up *.d.ts files from *.mjs. It only looks up *.d.mts files.

See

Notice that TypeScript can find types/src/pdf.d.ts even though it has the .d.ts extension because "types": "types/src/pdf.d.ts" is set in package.json.

@timvandermeij
Copy link
Contributor

timvandermeij commented Nov 11, 2023

The change itself looks good, but why didn't the typing tests catch this before when we did the change to JavaScript modules? I would assume that gulp typestest would have fallen over if the types can't be loaded (see also https://github.com/mozilla/pdf.js/tree/master/test/types), so would it be possible to adapt the tests so that they fail without this patch and succeed with this patch applied?

@tamuratak
Copy link
Contributor Author

I have added a test. Is it acceptable that the test depends on dist? It is much more reliable to test against the real target.

gulpfile.mjs Outdated Show resolved Hide resolved
gulpfile.mjs Outdated Show resolved Hide resolved
test/types/tsconfig.json Outdated Show resolved Hide resolved
test/types/package.json Outdated Show resolved Hide resolved
@tamuratak
Copy link
Contributor Author

Now the test depends on generic as before.

test/types/package-lock.json Outdated Show resolved Hide resolved
test/types/main.ts Outdated Show resolved Hide resolved
@Snuffleupagus

This comment was marked as resolved.

@timvandermeij
Copy link
Contributor

Yes, that should be correct because of the comment below it: type checking happens at compile time, not at run time, so the file itself is not actually run and therefore the exception is not actually thrown.

I have verified that the test do fail correctly if the type annotations are wrong, with the following diff:

diff --git a/test/types/legacy.ts b/test/types/legacy.ts
index 5a09b683d..7ed6b5805 100644
--- a/test/types/legacy.ts
+++ b/test/types/legacy.ts
@@ -2,7 +2,7 @@ import { getDocument } from "pdfjs-dist/legacy/build/pdf.mjs";
 import { EventBus } from "pdfjs-dist/legacy/web/pdf_viewer.mjs";
 
 class MainTest {
-  eventBus: EventBus;
+  eventBus: boolean;
   task: ReturnType<typeof getDocument> | undefined;
 
   constructor(public file: string) {
diff --git a/test/types/modern.ts b/test/types/modern.ts
index 06b892c91..c0720902f 100644
--- a/test/types/modern.ts
+++ b/test/types/modern.ts
@@ -2,7 +2,7 @@ import { getDocument } from "pdfjs-dist";
 import { EventBus } from "pdfjs-dist/web/pdf_viewer.mjs";
 
 class MainTest {
-  eventBus: EventBus;
+  eventBus: number;
   task: ReturnType<typeof getDocument> | undefined;
 
   constructor(public file: string) {

This results in the follow output with a non-zero exit code:

Couldn't compile TypeScript test:
test/types/legacy.ts(9,5): error TS2322: Type 'EventBus' is not assignable to type 'boolean'.
test/types/modern.ts(9,5): error TS2322: Type 'EventBus' is not assignable to type 'number'.

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, but please squash the commits before we can merge it. Thank you for fixing this and improving the tests!

@timvandermeij timvandermeij merged commit 2869b63 into mozilla:master Nov 12, 2023
5 checks passed
@timvandermeij
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Legacy: Type declarations are broken in 4.0
3 participants