Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

TypeScript: Jimp types are not corresponding to Jimp implementation #803

Closed
RomainFallet opened this issue Oct 13, 2019 · 31 comments · Fixed by #867
Closed

TypeScript: Jimp types are not corresponding to Jimp implementation #803

RomainFallet opened this issue Oct 13, 2019 · 31 comments · Fixed by #867
Labels
released This issue/pull request has been released.

Comments

@RomainFallet
Copy link

RomainFallet commented Oct 13, 2019

Expected Behavior

When importing Jimp as described in the doc for a TypeScript project:

import Jimp from 'jimp'

I should be able to use Jimp directly as is:

Jimp
  .read(fileData)
  .then((image: any) => {
    ...
  })
  .catch((err: any) => {
    ...
  })

Current Behavior

The compile step is OK, but there is runtime error saying:

TypeError: Cannot read property 'read' of undefined

If I use the require-like import syntax:

import * as Jimp from 'jimp'

and use Jimp as below:

Jimp
  .read(fileData)
  .then((image: any) => {
    ...
  })
  .catch((err: any) => {
    ...
  })

I then have a compile error saying:

Property 'read' does not exist on type 'typeof import("/path/to/node_modules/jimp/types/ts3.1/index")'

If I disable manually type checking for Jimp:

// @ts-ignore
Jimp
  .read(fileData)
  .then((image: any) => {
    ...
  })
  .catch((err: any) => {
    ...
  })

There is no runtime error and Jimp works as expected.

Failure Information (for bugs)

Context

  • Jimp Version: 0.8.4
  • Operating System: macOS 10.15 Catalina
  • Node version: 10.16.3
  • NPM Version: 6.11.3
  • TypeScript Version: 3.6.3

Workaround

To use Jimp with types information, I have to import the types separately and bind them to the classic require syntax:

import Jimp from 'jimp'
// tslint:disable-next-line: no-var-requires
const jimp: Jimp = require('jimp')

And then, use "jimp" instead of "Jimp":

jimp
  .read(fileData)
  .then((image: any) => {
    ...
  })
  .catch((err: any) => {
    ...
  })
@janikga
Copy link

janikga commented Oct 16, 2019

Thanks for raising the issue, I'm experiencing the same behaviour unfortunately :( From what I found when researching this, it looks like it is related to this issue #785

The worst part is that with the require syntax you cant type variables anymore, e.g.

const image: Jimp = await Jimp.read('someFilePath')

will raise an error 😢

@noomly
Copy link

noomly commented Oct 16, 2019

I'm also having an error on:

import Jimp from "jimp";

const myBmpBuffer: Buffer = loadDasBuffer();

Jimp.read(myBmpBuffer, (image: any) => {
    // something
});

which gives Argument of type 'Buffer' is not assignable to parameter of type 'number'; which basically renders the module quite unusable. Does anyone have a workaround?

@hipstersmoothie
Copy link
Collaborator

@crutchcorn

@crutchcorn
Copy link
Member

I sincerely think we should revert the export type to only export using namespace ala 0.6.4 typings. This doesn't mean we'd revert the 3.1 typings. I don't know enough about tsconfig options to provide an alternative.

@crutchcorn
Copy link
Member

I have no clue what's happening with @noomly example. I'd have to look further

@WeekendMan
Copy link

Same for me.
Types are completely fucked.
Documentation is completely fucked.
Examples from npm doc give problems with strict TS config

@rastographics
Copy link

With version 0.8.4, @RomainFallet workaround at the top works for me.
With version 0.8.5, the workaround no longer works. Errors given that jimp.read and jimp.AUTO are static members of type Jimp.

@rastographics
Copy link

Never mind. doesn't work with 0.8.4 either. Does anyone know any kind of workaround to get jimp to work in a typescript project now? I downgraded all the way to 0.5.3 still gives typescript errors.
Even if I lose ability for types...I just need to be able to build the project and run and client is not happy now. Will downgrading to a certain typescript version get this to work? Or what changed that caused the break? thank you

@rastographics
Copy link

Got this to work again by downgrading...
typescript down to 3.4.5
jimp down to 0.6.4
using import * as Jimp from 'jimp'; to import

@crutchcorn
Copy link
Member

I'm taking a look at fixing some of the type issues right now

@crutchcorn
Copy link
Member

@noomly it seems the bug you were experiencing was unrelated to the issue described in the OP. However, it's the first PR I've made for ;)

If you can test against 0.8.6-canary.810.481.0 it should include that fix

For everyone else, I have a solution to the overaching issues many users are facing since 0.6.4 in terms of typing problems. I'll have a PR open soon

@crutchcorn
Copy link
Member

I will also note to @WeekendMan:

I've been needing a break from working for OSS for a while because I tend to overwork myself. The only times I usually get to work on things is actually shown in your handle: The weekend, man.

Be patient and know that most of us working on OSS do so as a hobbyist project. Because it's open source, you're always able to contribute back upstream if you feel a fix is needed immediately

@taschmidt
Copy link

I had the strange issue that doing the simple import Jimp from 'jimp' was working until I ran my unit tests. I'm think this is due to webpack being able to avoid this problem somehow. However, I would run my unit tests (which are not webpack'd) and I'd get this problem which led me here.

After adding the const jimp: Jimp = require('jimp') hack, everything seemed to be working with my unit tests. However, this then broke the webpack side of things. So I had to create a super hack in the form of const jimp: Jimp = (Jimp || require('jimp')) and it seems to be working in both cases. Yuck!

@hipstersmoothie
Copy link
Collaborator

hipstersmoothie commented Nov 21, 2019 via email

@crutchcorn
Copy link
Member

crutchcorn commented Nov 21, 2019

@hipstersmoothie I don't think it needs to revert my changes, I think we need to modify the exports or improve the docs with TS usage. The fact that there's more than one working project with #815 and that #810 was quickly and able to be tested to work with one of the reported issues makes me believe that the changes I'd made as-of-now are no worse than the 0.6.5 release with the issues in them and significantly improved due to testing and more accurate overall tests

@crutchcorn
Copy link
Member

I've done the closest thing to "reverting" the changes from 0.6.5 that are causing many of the problems that people are reporting (and why so many are saying that 0.6.4 were the last working version for them):

#820

(I would ask folks to test the version of jimp associated with this PR but just my luck the release step failed)

We also need to bring in the PR:

#815

In order to fix the runtime errors that were caused unrelated to the TS definition changes in 0.8

So when testing the version above, if you notice runtime errors, that is fixed in a different PR

Lastly, @noomly's issues with the 0.8.5 defintion files (that were a regression from 0.8 from other versions) have been fixed in a PR for a while that just needs to be merged:

#810

@crutchcorn
Copy link
Member

Are y'all able to test against 0.9.2 stable release. There was some work done to improve TS stability and should fix everyone's issues with typings (if you were one of the ones that had to revert to 0.6.4, this includes you). Please test against and confirm/deny crosses fingers

@forivall
Copy link
Contributor

forivall commented Mar 27, 2020

🖐️ @crutchcorn I'm really good at writing typings, are you open to a large PoC PR on this?

@crutchcorn
Copy link
Member

crutchcorn commented Mar 27, 2020

Hi @forivall 👋 Small world!

What'd the PoC PR be in regards to? I've done a fair amount of work in order to modularize the typings for plugins and it seems to be in a fairly stable place now especially with the type tests that were written

To be clear, I'm not against it by any means - just wanted to check what I might've missed mentally beforehand :)

@crutchcorn
Copy link
Member

crutchcorn commented Mar 28, 2020

Speaking of... @hipstersmoothie, think we can close this issue out? I didn't realize it was still open and according to many users, it seems we're finally at a stable place

forivall added a commit to forivall/jimp that referenced this issue Mar 28, 2020
@hipstersmoothie
Copy link
Collaborator

Closed! Good job by the way. Now that the storm has settled it seems like all is good.

@forivall
Copy link
Contributor

forivall commented Mar 28, 2020

I was originally thinking of something along the lines of how the express typings in DefinitelyTyped does plugins, but then, by playing around in my node modules, I found a satisfactory solution: master...forivall:fix/split-ctor-instance-types

forivall added a commit to forivall/jimp that referenced this issue Mar 28, 2020
@forivall
Copy link
Contributor

wait, nvm, i've still got more tweaks to do.

@crutchcorn
Copy link
Member

Oh gotcha I see what you're up to! Love what you've been able to do thus far. If we're able to get some stuff into our type tests, I'd appreciate that as well! I think once you have more tweaks (as you'd mentioned, I didn't have any notes at a first glance), I'll be able to review the PR today and we can probably get it moved forward.

Thanks so much for jumping in and helping! :D

forivall added a commit to forivall/jimp that referenced this issue Mar 28, 2020
forivall added a commit to forivall/jimp that referenced this issue Mar 28, 2020
hipstersmoothie pushed a commit that referenced this issue Mar 30, 2020
* Properly split constructor and instance types

Fixes #803

* Fix a preexisting typo in Well Formed Plugin detection

* Also declare encoders and decoders types

* Update types tests

* Fix custom instance types

* Fix tsTest errors

* Fix typings for typescript 3.1
@hipstersmoothie
Copy link
Collaborator

🚀 Issue was released in v0.10.0 🚀

@hipstersmoothie hipstersmoothie added the released This issue/pull request has been released. label Mar 30, 2020
@hipstersmoothie
Copy link
Collaborator

🚀 Issue was released in v0.10.0 🚀

@ghost
Copy link

ghost commented Feb 25, 2024

Still having this issue, you could use the require statement to fix it but it's bad practice, no other library has this issue

To fix the "read is not defined" error you can do this

        const TypedJimp: Jimp & {
            read: Function
        } = require('jimp')

@crutchcorn
Copy link
Member

"no other library has this problem" lol ok. PRs welcome, I'll review them myself :)

@hipstersmoothie
Copy link
Collaborator

hipstersmoothie commented Feb 25, 2024

                       🕯
              🕯              🕯
        🕯                         🕯
 
   🕯            PRs welcome                🕯

        🕯                           🕯
              🕯              🕯
                        🕯

@MZawadz1
Copy link

For the record, I've just encountered this behaviour when updating jimp version - for me solution was to replace
import Jimp from "jimp";
with
import { Jimp } from "jimp";

So it's not the way documentation suggests.
Hope it helps someone :)

@hipstersmoothie
Copy link
Collaborator

Good catch. just pushed an update to the docs

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants