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

Add typescript type definitions for js lib #44

Closed
chris-si opened this issue Feb 16, 2024 · 11 comments
Closed

Add typescript type definitions for js lib #44

chris-si opened this issue Feb 16, 2024 · 11 comments
Assignees
Labels
invernizzi-attention Shortlisted for Luca js-package This issue is related to the javascript/npm package

Comments

@chris-si
Copy link

chris-si commented Feb 16, 2024

Hi,

it would be great if you could add or auto generate type definitions for the js lib so it can be used with typescript.

Since there isn't a CI job to handle automatic releases and I'm not aware of your release process and plans for the lib, I'd rather not myself create a PR for this yet.

@elhadjaoui
Copy link

facing the same issue :
Could not find a declaration file for module 'magika'. '/node_modules/magika/magika.js' implicitly has an 'any' type.

@reyammer reyammer added the js-package This issue is related to the javascript/npm package label Feb 19, 2024
@reyammer
Copy link
Collaborator

Thanks for reporting, looking into it.

@reyammer
Copy link
Collaborator

See #129, not sure if it's related.

@glassonion1
Copy link

glassonion1 commented Feb 26, 2024

I tried adding types to DefinitelyTyped to use Magika with TypeScript. However, I encountered the following error, and the tests did not pass. Does Magika not support CommonJS?

A require call resolved to an ESM JavaScript file, which is an error in Node and some bundlers.
CommonJS consumers will need to use a dynamic import.

DefinitelyTyped's pull request is below:
DefinitelyTyped/DefinitelyTyped#68765

@glassonion1
Copy link

glassonion1 commented Feb 27, 2024

@reyammer
It appears that Magica doesn't seem to support CommonJS, as shown in #129. This suggests that we should be able to add types to DefinitelyTyped.

module.exports = { Magika };

@glassonion1
Copy link

@chris-si
I have added type definitions for Magika to DefinitelyTyped. With this, you can now run it in TypeScript without any errors.

npm: https://www.npmjs.com/package/@types/magika

Please execute the following command.

npm install --save-dev @types/magika

or

pnpm add -D @types/magika

Additional information:
Magika did not support CommonJS and didn't pass the tests at DefinitelyTyped, the DefinitelyTyped team has been modified to allow the tests to pass even if the library does not support CommonJS.

@invernizzi
Copy link
Member

We've published a version with commonJS support, and the library has been rewritten in Typescript as of yesterday.
Our types should be bundled with the package. Please take a look and let me know if they are as expected.

@invernizzi invernizzi added the invernizzi-attention Shortlisted for Luca label Mar 7, 2024
@chris-si
Copy link
Author

chris-si commented Mar 7, 2024

That's great to hear! I'll give it a try in the coming days.

@glassonion1
Copy link

That's great. I tried 0.2.10 out right away.

I'm using Magika as a client application. While doing so, I encountered an issue concerning the argument types of the identifyBytesFull and identifyBytes methods.
The type of fileBytes being Uint16Array | Buffer required the following configuration in webpack.

// webpack.config.js
module.exports = {
  //...
  resolve: {
    fallback: {
      fs: false
    }
  },
};

Buffer is a type specific to Node.js and is not suitable for use in the browser. Additionally, for files used in the browser, the type Uint8Array is more commonly used instead of Uint16Array.
I would like the type of fileBytes to be changed to Uint8Array for comfortable use in a browser environment.

@invernizzi
Copy link
Member

Thanks for the quick check! Agreed on both points. 0.2.12 accepts Uint8Array and drops Buffer for non-Node magika.

@chris-si
Copy link
Author

I tried it out and it works like expected! Thanks for improving the js/ts lib!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invernizzi-attention Shortlisted for Luca js-package This issue is related to the javascript/npm package
Projects
None yet
Development

No branches or pull requests

5 participants