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

Try to fix TypeScript definitions for the es5-build in pdfjs-dist (issue 12872) #12834

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

Snuffleupagus
Copy link
Collaborator

Given that we're already using the external/dist/ folder for things we simply want to copy to pdfjs-dist during building, this patch should hopefully work since it's based on the suggestion in #12827 (comment).

As long as this only requires a single/small file, to fix the TypeScript definitions in es5-builds, this solution seem acceptable as far as I'm concerned. (Although, please note that I don't know enough about TypeScript to actually test the patch.)

Fixes #12827

…sue 12872)

Given that we're already using the `external/dist/` folder for things we simply want to copy to pdfjs-dist during building, this patch *should* hopefully work since it's based on the suggestion in issue 12827.

As long as this only requires a *single/small* file, to fix the TypeScript definitions in es5-builds, this solution seem acceptable as far as I'm concerned. (Although, please note that I don't know enough about TypeScript to actually test the patch.)
@timvandermeij timvandermeij merged commit 6089b8f into mozilla:master Jan 8, 2021
@timvandermeij
Copy link
Contributor

Let's do this; thanks!

@Snuffleupagus Snuffleupagus deleted the es5-types branch January 8, 2021 21:34
@simonhaenisch
Copy link

Somehow this file is not part of pdfjs-dist, or at least not anymore:

❯ ls node_modules/pdfjs-dist/es5/build
pdf.js
pdf.js.map
pdf.min.js
pdf.worker.entry.js
pdf.worker.js
pdf.worker.js.map
pdf.worker.min.js

(see https://github.com/mozilla/pdfjs-dist/tree/7bfceb110b9b27a09cc3d69f4907c59fd5561f0c/es5/build)

I couldn't figure out where the build script for pdfjs-dist lives but is it possible that it's just forgetting to include this file/file extension in the output?


FWIW, re the original issue #12827: there's also the official browser field in package.json (see https://docs.npmjs.com/cli/v7/configuring-npm/package-json#browser), so it could be

{
  "main": "es5/build/pdf.js",
  "browser": "build/pdf.js",
  "types": "types/pdf.d.ts",
  "//": "..."
}

However I'm not totally sure about the implications of this, i. e. how well browser is supported by other tools (should be pretty good support though).

@simonhaenisch
Copy link

Hm actually never mind I just figured out that the latest stable release (2.6.347) was from 5 months ago... the file is there now that I installed pdfjs-dist@next (2.7.570).

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.

Typescript types definition missing for es5 build
3 participants