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

[Request for help from TypeScript users] Re-enable gulp typestest in GitHub Actions #16705

Closed
Snuffleupagus opened this issue Jul 18, 2023 · 7 comments · Fixed by #16890
Closed
Labels

Comments

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jul 18, 2023

PLEASE NOTE: There's a risk that the type-definitions themselves are also broken now (and not just the tests), which means that unless this is fixed very soon the next PDF.js release may not have working types.


This was temporary disabled in PR #16698, and should be re-enabled as soon as possible.

  1. If you're new to the project, please see https://github.com/mozilla/pdf.js#getting-the-code and https://github.com/mozilla/pdf.js/wiki/Contributing
  2. Revert commit d9350c3 locally, using git revert d9350c3 --no-edit.
  3. Update the following code to handle (at least) the new display-svg import:

    pdf.js/gulpfile.mjs

    Lines 1531 to 1548 in 43fc788

    gulp.task("types", function (done) {
    console.log("### Generating TypeScript definitions using `tsc`");
    const args = [
    "target ESNext",
    "allowJS",
    "declaration",
    `outDir ${TYPES_DIR}`,
    "strict",
    "esModuleInterop",
    "forceConsistentCasingInFileNames",
    "emitDeclarationOnly",
    "moduleResolution node",
    ].join(" --");
    exec(
    `"node_modules/.bin/tsc" --${args} src/pdf.js web/pdf_viewer.component.js`,
    done
    );
    });
  4. Run gulp typestest, and gulp ci-test, to validate your changes.
  5. Finally, ensure that both gulp jsdoc and gulp web still work.
@rafagsiqueira

This comment was marked as duplicate.

tamuratak added a commit to tamuratak/pdf.js that referenced this issue Aug 6, 2023
tamuratak added a commit to tamuratak/pdf.js that referenced this issue Aug 6, 2023
@tamuratak
Copy link
Contributor

One of the available solutions is using typedoc instead of jsdoc since typedoc supports annotations like @typedef { import("./svg.js").SVGGraphics } SVGGraphics. The output HTML files seem better informative.

Is it feasible switching to typedoc?

tamuratak added a commit to tamuratak/pdf.js that referenced this issue Aug 6, 2023
tamuratak added a commit to tamuratak/pdf.js that referenced this issue Aug 6, 2023
@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Aug 6, 2023

Is it feasible switching to typedoc?

Sorry, but as far as I'm concerned that's not a "correct" solution since it'd do nothing to address the underlying problem.

Please note that the actual issue here is that the type-generation currently doesn't understand our (development mode) import maps, which is what we really want to address here. Otherwise the same kind of issue could very easily resurface elsewhere in the code-base e.g. the next week/month, and a solution that'd require adding even more TypeScript-specific comments to the code-base will further increase the maintenance burden of the types.

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Aug 6, 2023

Quickly skimming through the TypeScript documentation I found the following, which seems relevant here: https://www.typescriptlang.org/tsconfig/#paths

@tamuratak
Copy link
Contributor

tamuratak commented Aug 8, 2023

Even with the "paths" option, tsc doesn't rewrite the module paths in the generated declaration files. See microsoft/TypeScript#10866 (comment)

Node.js supports a importmap-like feature. You define the mapping directly in the package.json file.:

And TypeScript works with the feature:

However, based on my experiments, it is seems difficult to make the feature work with the PDF.js library since PDF.js distributed in the AMD format. The feature works best with libraries that are in ESM formats.

So, I think switching to typedoc is the most feasible fix. If it is not acceptable due to the maintenance burden, removing the SVG back-end, #16699, would be the simplest one. Until the PR merged, it would be better to tell users to set skipLibCheck: true in their tsconfig.json, which suppresses the TypeScript error.

@Snuffleupagus
Copy link
Collaborator Author

So, I think switching to typedoc is the most feasible fix.

Personally I don't like the idea of the type-generation deciding how we can/can't write code and/or what dependencies we use. Please keep in mind that maintaining the type-generation has already taken a lot of time/effort for unpaid contributors (such as myself), and I'd really like to avoid adding to this by further complicating things here.

Given that the primary development target for the PDF.js library is the built-in Firefox PDF Viewer, keeping the library working in other browsers/environments takes much more time/effort than many users probably realize; hence why I really want to avoid increasing the maintenance burden "just" for something like type-generation.

removing the SVG back-end, #16699, would be the simplest one.

First of all, it's not clear if/when that'll happen. Secondly it'd unfortunately not really fix anything, but only work-around the current issue, since the same underlying problem could resurface somewhere else next week.

Until the PR merged, it would be better to tell users to set skipLibCheck: true in their tsconfig.json, which suppresses the TypeScript error.

That seems like a good work-around for now; thank you!

@franklbt
Copy link

franklbt commented Sep 5, 2023

Seems not solved in the latest version :

X [ERROR] TS2304: Cannot find name 'SVGGraphics'. [plugin angular-compiler]

node_modules/pdfjs-dist/types/src/display/api.d.ts:1466:9:
  1466 │ export { SVGGraphics };

pdfjs-dist: ^3.10.111
Using this library with angular v16 project

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