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

It is not supporting the TS type declaration. #135

Open
Basavarajrp opened this issue Jul 23, 2023 · 25 comments
Open

It is not supporting the TS type declaration. #135

Basavarajrp opened this issue Jul 23, 2023 · 25 comments

Comments

@Basavarajrp
Copy link
Contributor

Describe the bug
Could not find a declaration file for module 'pdf reader'

To Reproduce

  1. I am using Next JS application with TS support, and getting an error as Could not find a declaration file for module 'pdf reader'.
    import { PdfReader } from "pdfreader";
  2. Also tried to look for the Type definition file on yarn packages but was not found.
Screenshot 2023-07-23 at 11 11 34 AM

Tq 🙃.

@adrienjoly
Copy link
Owner

Hi, thanks for the heads up!

There are currently no TypeScript types defined for pdfreader. But that would be a nice addition indeed!

I'm guessing that writing a index.d.td file in this repo and linking package.json to it would do.

@Basavarajrp, would you be interested in doing it and proposing this evolution as a Pull Request? I can help if needed.

@Basavarajrp
Copy link
Contributor Author

Hi @adrienjoly ,

I'm excited to contribute to this repository by adding the type declaration files. I appreciate your help and guidance on where to start and how to get things going.

😊 Thank you!

@adrienjoly
Copy link
Owner

Awesome!

Here's the official procedure on how to add type declarations in a npm package: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#including-declarations-in-your-npm-package

Here is the steps to follow in order to contribute that change to pdfreader's github repository:

  1. fork the repository to your own github account => git clone it from there to your computer
  2. add the type declarations, and make sure that they work as expected (e.g. from a test project that imports your version of pdfreader)
  3. commit the changes and create the pull request back to this github repository

Let me know if your need assistance on any of these steps.

@Basavarajrp
Copy link
Contributor Author

Basavarajrp commented Aug 8, 2023

Hi @adrienjoly ,

Thank you for this guidance, I will work on it and get back to you if any help is needed.

Tq 😊.

@adrienjoly
Copy link
Owner

This paragraph may be helpful:

With TypeScript’s original Node support, it would look for a "main" field, and then look for declaration files that corresponded to that entry. For example, if "main" pointed to ./lib/index.js, TypeScript would look for a file called ./lib/index.d.ts. A package author could override this by specifying a separate field called "types" (e.g. "types": "./types/index.d.ts").

Cf TypeScript: Documentation - ECMAScript Modules in Node.js

@niemal
Copy link
Contributor

niemal commented Aug 12, 2023

I wrote a very weak mockup since I am interested in using just those 2 functions (parseFileItems, parseBuffer), maybe you guys can pick up from it and make it full:

declare module "pdfreader" {
  type InitOptions = { password?: string; debug?: boolean };
  type Error = null | string;

  type DataEntry = {
    page?: number;
    width?: number;
    height?: number;
    text?: string;
    file?: {
      path?: string;
      buffer?: string;
    };
  } | null;

  type ItemHandler = (err: Error, data: DataEntry) => void;

  class PdfReader {
    constructor(opts: InitOptions | null): PdfReader;
    parseFileItems(pdfFilePath: string, itemHandler: ItemHandler): void;
    parseBuffer(buffer: Buffer, itemHandler: ItemHandler): void;
  }
}

Right now I am facing this issue: modesty/pdf2json#303

@Basavarajrp
Copy link
Contributor Author

Basavarajrp commented Aug 12, 2023

Hi @niemal and @adrienjoly

I started working on this, I'm wondering whether I should create a separate project and publish it as an npm package. Alternatively, should I include declarations within the same repository?

I've noticed various projects that maintain distinct repositories solely for type declarations.

Please provide your guidance and insights.

Also, I heard about JSDoc type annotations

According to the previous conversation with @adrienjoly I think I need to follow this following stpes.
Got it! If you have a JavaScript-only project and the maintainers are asking you to add type declarations and publish it as an npm package, you can use the @types organization on npm to publish the type declarations for the existing JavaScript codebase. This way, developers who use TypeScript can benefit from type hints and better tooling when using your package.

Here's what you can do:

  1. Create TypeScript Declaration Files:

    • For each JavaScript file in the project, create a corresponding TypeScript declaration (.d.ts) file. These files will contain type declarations that match the structure of the JavaScript code.
    • The declaration files should be placed in a separate directory, often named types or typings, within the project's root directory.
  2. Add Type Declarations:

    • In each .d.ts file, provide type annotations for the functions, variables, and classes defined in the corresponding JavaScript file. You can use JSDoc-style comments to document the types.
  3. Publish Type Declarations to npm:

    • Publish the type declarations as a separate npm package with a name like @types/your-package-name.
    • You can publish type declarations using the following command:
      npm publish --access public
  4. Update the JavaScript Package:

    • In the package.json file of your JavaScript project, make sure you have a "types" field pointing to the location of your type declaration directory. For example:
      "types": "types/index.d.ts"

@niemal
Copy link
Contributor

niemal commented Aug 12, 2023

I edited the above mockup to the following, seems to be more ideal to the way things are exported. I am trying to make it work with Next.js so I will get back to you with the working version when that happens. For the moment I cloned both of this repo and pdf2json one, importing them internally and see what works and what not.

This is index.d.ts file inside the root directory of npm-pdfreader:

export type InitOptions = { password?: string; debug?: boolean };
export type Error = null | string;

export type DataEntry = {
  page?: number;
  width?: number;
  height?: number;
  text?: string;
  file?: {
    path?: string;
    buffer?: string;
  };
} | null;

export type ItemHandler = (err: Error, data: DataEntry) => void;

export declare class PdfReader {
  constructor(opts: InitOptions | null): PdfReader;
  parseFileItems(pdfFilePath: string, itemHandler: ItemHandler): void;
  parseBuffer(buffer: Buffer, itemHandler: ItemHandler): void;
}

@Basavarajrp
Copy link
Contributor Author

@niemal , are you working on this? Or should I continue working on this? I am happy to work on this.

@niemal
Copy link
Contributor

niemal commented Aug 12, 2023

@niemal , are you working on this? Or should I continue working on this? I am happy to work on this.

You should continue to work on this, I am working on something else but I am happy to provide any input I can.

@niemal
Copy link
Contributor

niemal commented Aug 12, 2023

This is index.d.ts file inside the root directory of npm-pdfreader:

export type InitOptions = { password?: string; debug?: boolean };
export type Error = null | string;

export type DataEntry = {
  page?: number;
  width?: number;
  height?: number;
  text?: string;
  file?: {
    path?: string;
    buffer?: string;
  };
} | null;

export type ItemHandler = (err: Error, data: DataEntry) => void;

export declare class PdfReader {
  constructor(opts: InitOptions | null): PdfReader;
  parseFileItems(pdfFilePath: string, itemHandler: ItemHandler): void;
  parseBuffer(buffer: Buffer, itemHandler: ItemHandler): void;
}

This worked successfully after using a static folder with a stand-alone pdf2json as a module in Next.js and importing that stand-alone pdf2json to use. You will probably want to use the above file as a basis to further write-up all the classes that are exported (except PdfReader).

@adrienjoly
Copy link
Owner

adrienjoly commented Aug 13, 2023 via email

@niemal
Copy link
Contributor

niemal commented Aug 13, 2023

Good job! Would you be interested to contribute that file into pdfreader’s official repository, by sending a pull request? Let me know if you want assistance in doing that. Adrien Le sam. 12 août 2023 à 16:57, Nick C. @.> a écrit :
This is index.d.ts file inside the root directory of npm-pdfreader: export type InitOptions = { password?: string; debug?: boolean };export type Error = null | string; export type DataEntry = { page?: number; width?: number; height?: number; text?: string; file?: { path?: string; buffer?: string; };} | null; export type ItemHandler = (err: Error, data: DataEntry) => void; export declare class PdfReader { constructor(opts: InitOptions | null): PdfReader; parseFileItems(pdfFilePath: string, itemHandler: ItemHandler): void; parseBuffer(buffer: Buffer, itemHandler: ItemHandler): void;} This worked successfully after using a static folder with a stand-alone pdf2json as a module in Next.js and importing that stand-alone pdf2json to use. You will probably want to use the above file as a basis to further write-up all the classes that are exported (except PdfReader). — Reply to this email directly, view it on GitHub <#135 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEB2RJ3YG2FYPK2LEK4KOTXU6KU3ANCNFSM6AAAAAA2UJJOKA . You are receiving this because you were mentioned.Message ID: @.
>
-- Envoyé avec Gmail Mobile

#137

@adrienjoly
Copy link
Owner

Version 3.0.1 includes the type definitions your proposed in your pull request. Thank you for your contribution!

@Basavarajrp
Copy link
Contributor Author

Basavarajrp commented Aug 15, 2023

Hello Team (@adrienjoly and @niemal),

I have created the type declaration files for all the modules and here you can check them. master...Basavarajrp:npm-pdfreader:master

"Please let me know if I'm heading in the right direction. While I still need to test and refine this further, I would appreciate your input on whether my current progress is on track."

@niemal
Copy link
Contributor

niemal commented Aug 15, 2023

Hello Team (@adrienjoly and @niemal),

I have created the type declaration files for all the modules and here you can check them. master...Basavarajrp:npm-pdfreader:master

"Please let me know if I'm heading in the right direction. While I still need to test and refine this further, I would appreciate your input on whether my current progress is on track."

This looks pretty solid on first sight! Have you tested it on a typescript project? Regarding Next.js, the problem is with pdf2json not being imported as a stand-alone module so what worked for me was having it in a directory exported as a module and have pdfreader using that (I didn't npm install either of those).

I will see if I can squeeze some time to test this but if you have tested it using it in a typescript project then you should certainly make a pull request and delete my own code (index.d.ts).

@Basavarajrp
Copy link
Contributor Author

Hello Team (@adrienjoly and @niemal),
I have created the type declaration files for all the modules and here you can check them. master...Basavarajrp:npm-pdfreader:master
"Please let me know if I'm heading in the right direction. While I still need to test and refine this further, I would appreciate your input on whether my current progress is on track."

This looks pretty solid on first sight! Have you tested it on a typescript project? Regarding Next.js, the problem is with pdf2json not being imported as a stand-alone module so what worked for me was having it in a directory exported as a module and have pdfreader using that (I didn't npm install either of those).

I will see if I can squeeze some time to test this but if you have tested it using it in a typescript project then you should certainly make a pull request and delete my own code (index.d.ts).

Now I have not tested completely, but when I tried I was not getting any type specific errors while using Boolean values instead string just to test. So I need help on this testing I will continue more on this on my next work cycle.

@Basavarajrp
Copy link
Contributor Author

Basavarajrp commented Aug 15, 2023

I've created the test.ts file within the test folder. When I move the .d.ts files outside the types folder (to the root directory), the .ts files start generating type mismatch errors. However, if I keep all the .d.ts files inside the types folder, I don't encounter any type mismatch errors.

This is my tsconfig.json file: Any corrections needed here.

{
  "compilerOptions": {
    "allowJs": true,
    "checkJs": true,
    "declaration": true,
    "declarationDir": "./types",
    "target": "ES6"
  },
  "include": ["types/**/*.d.ts"],
  "exclude": ["node_modules"]
}

This is the problem I am stuck with now.

cc: @adrienjoly and @niemal

@Basavarajrp
Copy link
Contributor Author

Basavarajrp commented Aug 15, 2023

Hello @niemal,

I wanted to let you know that I've implemented a workaround for the issue we discussed. I created an index.d.ts file within the types folder and exported the essential type declarations from there. Furthermore, I made the necessary adjustments to the tsconfig.json file. This approach successfully resolved the problem I initially mentioned.

@niemal, could you kindly take a look at this pull request? I'd appreciate it if you could confirm whether the issue you raised has also been resolved now with this implmentation.

PR: https://github.com/adrienjoly/npm-pdfreader/pull/139/files#diff-093ad82a25aee498b11febf1cdcb6546e4d223ffcb49ed69cc275ac27ce0ccce

Branch : Basavarajrp:configureTSDeclaration

🙃 Thank you for your time and assistance.

@niemal
Copy link
Contributor

niemal commented Aug 16, 2023

Hello @niemal,

I wanted to let you know that I've implemented a workaround for the issue we discussed. I created an index.d.ts file within the types folder and exported the essential type declarations from there. Furthermore, I made the necessary adjustments to the tsconfig.json file. This approach successfully resolved the problem I initially mentioned.

@niemal, could you kindly take a look at this pull request? I'd appreciate it if you could confirm whether the issue you raised has also been resolved now with this implmentation.

PR: https://github.com/adrienjoly/npm-pdfreader/pull/139/files#diff-093ad82a25aee498b11febf1cdcb6546e4d223ffcb49ed69cc275ac27ce0ccce

Branch : Basavarajrp:configureTSDeclaration

upside_down_face Thank you for your time and assistance.

Hey,

First off the issue I mentioned is not an issue of this package but pdf2json and no it didn't resolve, unfortunately. I think we should implement a proper full test.ts file which compensates and imitates test.js respectively. I think we should also hear from @adrienjoly for a more experienced and in-depth opinion.

Cheers!

@adrienjoly adrienjoly reopened this Aug 16, 2023
@adrienjoly
Copy link
Owner

Hi! Can you summarize and clarify what objective we're targeting here?

What do you want to be able to do with these type definitions, exactly?

@Basavarajrp
Copy link
Contributor Author

Hi! Can you summarize and clarify what objective we're targeting here?

What do you want to be able to do with these type definitions, exactly?

We have decided to include type definitions for the entire project. Since I opened this issue to address the lack of TypeScript support in the package, I have taken the initiative to add the TypeScript type definition files to provide the necessary support.
So I was working on it, to add the TS support for this project.

@adrienjoly
Copy link
Owner

Ok.
I think it's nice to have TypeScript types in the codebase but I'm currently on vacation, so I'd like to minimize my time spent reviewing and and merging pull requests.

So far, I've already merged 2 PRs related to typing, in less than a week. => I'd appreciate it if you could converge to a final PR that you both tested and agree on. I'll be happy to review and merge it when you're fully done with it. To clarify, my hope is to not have to review yet another PR about typing after merging that one.

If you have specific questions, I'll do my best to answer them.

Thank you for your understanding.

Adrien

@adrienjoly
Copy link
Owner

Just in case it was not clear in my last message: feel free to keep discussing and collaborating together here if it's convenient for you. Let me know when your PR is ready for review.

Thank you! ^^

Adrien

@zozo-yasuda
Copy link
Contributor

Hello everyone, I made a pull request for type definitions for the TableParser class. Please feel free to point out improvements I could make to this.
I especially was not too sure about what data type "item" should be, as the existing code seemed to suggest the DataEntry type but the text object that the code produced seemed to not have the same properties as the DataEntry class.

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

No branches or pull requests

4 participants