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

Please include TypeScript types in the package itself #899

Closed
2 tasks done
dantman opened this issue Nov 30, 2021 · 10 comments
Closed
2 tasks done

Please include TypeScript types in the package itself #899

dantman opened this issue Nov 30, 2021 · 10 comments
Labels
enhancement New feature or request stale wontfix This will not be worked on

Comments

@dantman
Copy link

dantman commented Nov 30, 2021

Before you start - checklist

  • I understand that React-PDF does not aim to be a fully-fledged PDF viewer and is only a tool to make one
  • I have checked if this feature request is not already reported

Description

Currently TypeScript types are provided by an external @types/react-pdf package. This allows the types definition to get out of sync with the react-pdf library.

Normally this wouldn't be an issue, except currently a mistake in the types package actually breaks react-pdf itself.

  • react-pdf uses "pdfjs-dist": "2.9.359",
  • @types/react-pdf uses "pdfjs-dist": "^2.10.377"

As a result it's possible that node_modules/pdfjs-dist may be v2.10.377 while react-pdf uses v2.9.359. Because all of the PDF worker setup methods documented by react-pdf use node_modules/pdfjs-dist, this means it's possible that simply installing @types/react-pdf can result in the pdfjs worker installed and the pdfjs library react-pdf uses being of different versions.

Proposed solution

Including the TypeScript types as part of the react-pdf package itself will permanently fix this issue by avoiding the duplication of the very specific dependency version.

  • A) The .d.ts files from the @types/react-pdf package can be moved to react-pdf's codebase and maintained alongside it
  • B) Using those .d.ts files, react-pdf could be refactored to TypeScript .tsx files so react-pdf is written in TypeSript.

Alternatives

Yes, it is possible to fix the @types/react-pdf package for the current version (though the publishing method means this will likely take several days or even a week.

However even if we fix it now. Because react-pdf pins an exact version of pdfjs the next time react-pdf updates its version of pdfjs (which is already outdated) this bug will regress and will need to be fixed again. Though this will likely not happen in a timely manor as someone will have to discover and re-investigate this bug (which only shows up at runtime) before submitting a PR to update the pdfjs version in the types package to match.


One other possibility would be to use peerDeps instead of deps for the pdfjs package. This would ensure that pdfjs is installed at the root, with the correct version, and it matches the version that react-pdf expects.

Additional information

No response

@dantman dantman added the enhancement New feature or request label Nov 30, 2021
@wojtekmaj
Copy link
Owner

wojtekmaj commented Nov 30, 2021

I'm all for refactoring React-PDF to TypeScript actually, but frankly, I don't even know how to start.

Regardless, I suggest raising an issue in DefinitelyTyped repo to resolve the issue for now.

@wojtekmaj wojtekmaj added the help wanted Extra attention is needed label Nov 30, 2021
@dantman
Copy link
Author

dantman commented Nov 30, 2021

Yes, it is possible to fix the @types/react-pdf package for the current version (though the publishing method means this will likely take several days or even a week.

Regardless, I suggest raising an issue in DefinitelyTyped repo to resolve the issue for now.

Actually I just went to write a PR to fix the issue. However it appears this is not fixable on the DefinitelyTyped side. It appears that the TypeScript types for [email protected] are missing a type (onLoadSuccess in which the pdf arg is supposedly a PDFDocumentProxy). So downgrading the pdfjs version used by the types package actually breaks the types package.

So fixing the types package may actually require react-pdf to upgrade its version of pdfjs-dist first.

@dantman
Copy link
Author

dantman commented Nov 30, 2021

I'm all for refactoring React-PDF to TypeScript actually, but frankly, I don't even know how to start.

Maybe it's possible to do this incrementally.

  1. integrate .d.ts files into the package
  2. integrate tsc into the build process, using it as part of the JS compilation process (IIRC tsc can do some compilation of JS as well as TS)
  3. file by file port the .d.ts and .jsx file pairs into .tsx files

@dantman
Copy link
Author

dantman commented Nov 30, 2021

I'm all for refactoring React-PDF to TypeScript actually, but frankly, I don't even know how to start.

My client is open to giving me 2 days to spend on refactoring react-pdf to TypeScript, with a bit of interruption for higher priority stuff. Not sure I can finish a full refactor in that time but it should give you a start.

You could start by upgrading pdfjs-dist to 2.10.377 or 2.11.338. That would workaround the @types issue temporarily. And it'll be a pre-requisite for me refactoring the code to TypeScript since the @types code depends on a type missing in 2.9.x.

@wojtekmaj
Copy link
Owner

wojtekmaj commented Dec 1, 2021

2.10.377 is definitely on my radar! Missed that release being marked as stable. Thanks for the heads up.

Tracked under #900.

@shye0000
Copy link

shye0000 commented Dec 10, 2021

I also ran into the same issue about the unalignment of pdfjs-dist version when doing a full reinstall of my project's deps after upgrade to npm 8 without the package-lock.
a small workaround can do the magic:
First install the react-pdf with the other deps then install the @types/react-pdf manually.
Hope it can help if anyone have the same issue.
(Apparently the order of the installation tricks npm to resolve the deps tree differently)

@wojtekmaj
Copy link
Owner

wojtekmaj commented Dec 13, 2021

Note: v5.6.0 is now released, with pdfjs-dist bumped to 2.10.377.

@saadq
Copy link
Contributor

saadq commented Dec 18, 2021

For anyone else who is having issues due to being unable to import the PDFDocumentProxy type, you can just use a workaround like:

import { Document, DocumentProps } from 'react-pdf'

type LoadCallback = Required<DocumentProps>['onLoadSuccess']

function Display() {
  const onLoad: LoadCallback = useCallback((pdf) => {
    console.log(pdf.numPages) // `pdf` is properly typed here
    ...
  }, [])

  return (
    <Document loading="" file={url} onLoadSuccess={onLoad}>
      ...
    </Document>
  )
}

@github-actions
Copy link
Contributor

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this issue will be closed in 14 days.

@github-actions github-actions bot added the stale label Apr 25, 2022
@wojtekmaj wojtekmaj added wontfix This will not be worked on and removed help wanted Extra attention is needed labels Apr 27, 2022
@dwilt
Copy link

dwilt commented Oct 31, 2022

For those of you who actually need to access PDFDocumentProxy, building off of @saadq's approach:

type LoadCallback = Required<DocumentProps>['onLoadSuccess']
type PDFDocumentProxy = Parameters<LoadCallback>[0]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

6 participants
@dantman @dwilt @wojtekmaj @saadq @shye0000 and others