Fix issues with typings using classes, publish @core typings, and fix 3.1 typings

[crutchcorn]

What's Changing and Why

Copied from #785 (comment)

When doing testing for #786 , in order to avoid issues with yarn link I copied + pasted the packages folder. As a result, I forgot to update the npm whitelist and both @jimp/core and jimp do not copy their types folder to the final build, thus causing the error above. I was able to repro the issue and resolve it only by copying the types folder of each into their respective node_modules folder.

Unrelated notes on TS that I noticed while debugging this PR

There were two more things that I wanted to bring up here (just to keep everyone up-to-date, neither of these issues are breaking or should be handled now):

1. Not introduced in 0.8 - require no longer works for typings. This may have been noticed by @hipstersmoothie even when trying to fix tests in 0.6: 7dff287#diff-9687f7d238d1b6cbaf5bddfd10b57491L5

The reason const Jimp = require('jimp') doesn't work with types anymore is because now it's under Jimp.default. Now, if we wanted to enable this functionality again, we could easily change index.d.ts file from jimp like so:

export = Jimp;

But then the problem is that, as the TS language extensions will tell you, you cannot export like this and have any other exports. To get around this in the past, we used namespaces:
https://github.com/oliver-moran/jimp/blob/14055cf60e66701987bb148a2d389844dd0e9cb4/packages/jimp/jimp.d.ts

So if we're wanting to restore said functionality, we'd need to do the same thing today. Otherwise, if we're okay with it being as it is (the import works just fine), we should likely remove the require example from the README

2. There seems to have been an invisible requirement developed for these types: @types/node. Now, again, this was not introduced in 0.8 and it's actually somewhat hard to pinpoint when this was introduced (the type in question that it relies on is Buffer. Now, there's a few things we could do here:

• Introduce a peer dependency for the @types/node typings to inform the user that we expect them to have it installed
• Copy + paste the Buffer type ~ due to (seemingly) only having one type from that package we depend on we could likely just copy the type to be local. The issue here is that maintanance and type interop are a big problem - I'd highly suggest against this one
• Do nothing ~ I've seen a lot of typings simply assume that these types are installed to not enforce a peer dependency on non-TS users

Published PR with canary version: 0.8.2-canary.792.360.0

[amirburbea]

Worked

[crutchcorn]

Please refer to comment:

#785 (comment)

Why this was closed

[amirburbea]

I think because 0.8.1 didn't include them at all. So while the types kind of suck currently, at least they're included

…

On Thu, Sep 12, 2019, 1:12 PM Corbin Crutchley ***@***.***> wrote: Reopened #792 <#792>. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#792>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQGPCSYG5CNIK67SEG4D5DQJJZ77ANCNFSM4IWF5LVQ> .

[crutchcorn]

This PR removes the new typings for 3.1 from being exposed to TS and reverts architectural changes in jimp typings made that differed from 0.7's typings (outside of adding new methods/etc from the plugins). It does not remove the code (so when I come back in a week or two, I'll be able to make a PR instead of doing some odd rebase) and still allows configure to move forward and have functionality

TLDR It works for now ™️

[hipstersmoothie]

@crutchcorn Are you not moving forward with this? I want to get you to a point where you don't have sleepless nights ❤️

[crutchcorn]

@hipstersmoothie I'll be back in a week or so to take another stab at this (3.1 and otherwise). I think this PR is still good (at least the typings for <3.1 TS), but just needs to export using this:

https://github.com/oliver-moran/jimp/blob/7c9d3c817cade88d4a20422be10670d3c1528429/packages/jimp/jimp.d.ts#L2

Because it's using export = , you're unable to export anything else, but if you want to export those other items as well you'll need to add back in the namespacing removed in this commit: 466b65b

Either way, I'll be back eventually.

If this PR wants to be opened again before then and you (@hipstersmoothie) wants to commit directly to this PR, I am allowing maintainers to make edits. Otherwise, if work wants to be done before then and someone else wants to fork off of my fork and go from there please feel free

[crutchcorn]

The previous commit HAS fixed one of the larger issues with the code, but tomorrow I need to have a more thorough discussion with @hipstersmoothie regraurding supported. I'll be sending an email tomorrow morning (PDT) about that

[crutchcorn]

Copying from an email I sent to @hipstersmoothie:

There's some questions I had regarding what the supported method of import or require we want to enable for the TS typings. It's kinda the only question left for the typings AFAIK (Obviously we'll want more users to do testing, but with my most recent commit, the const A: Jimp = new Jimp('test') now works (for 3.1 and <3.1)

It seems there's also been a bit of API churn in this way as well:
357bd5a
7dff287
466b65b
#581
#660
#785 (comment)

So then, the questions are:

• Is const Jimp = require('jimp') allowed?
  (If yes, we'll need to export = Jimp)
• If so, should we also be able to import {PrintableText} from 'jimp'?
  (If yes and the previous question is also yes, we'll have to go back to namespacing the way prior to 660 being merged)
• If we DON'T care about require use, do we want to enable:
  import * as Jimp from 'jimp' or import Jimp from 'jimp?
  (We can enable both of these depending on the esModuleInterop and allowSyntheticDefaultImports values of a user's TSConfig)

I personally think we should not allow const Jimp = require('jimp') for TS typings and enforce import Jimp

Regardless of our choice, we should edit the README. I know how to achieve the types of all of these, but I just need to know which route we're wanting to go. Keep in mind, these answers will impact how we handle our tsconfig.js

[crutchcorn]

After the export/import answers are resolved, I believe (tests witholding) I have TS (all versions) fully working with class usage/non class usage/etc. The tests are now very lengthy and contain every usecase I could come accross.

[hipstersmoothie]

const Jimp = require('jimp')

This just has to work in plain JS. In TS i'm fine if that doesn't work

[crutchcorn]

I've made the ability to use require('jimp') and import * as Jimp from 'jimp not valid in the TS typings anymore.

Aside: I have reason to believe they were both already removed in 0.6.4 without being announced as such, which is why we have users saying their typings did not work on anything higher, but that's another story

As a result, I have updated the docs in the README regarding the change. We should probably also mention it SOMEHOW. I don't know if that means a minor bump and warning TS users that 0.8 is no good or if it's just mentioned in a release note that 0.6.4 introduced this/etc, but it's there. I am going to ask the users in the issue thread about testing the most recent published canary with the caviat that neither of these old imports methods work in TS (but work just as they would usually in vanilla JS)

[amirburbea]

I assume you know this, but generally if allowSyntheticDefaultImports is on, the way to load it should be to use import jimp from 'jimp' (otherwise if the flag is off import * as jimp from 'jimp').

Microsoft recommends projects to aim to have the flag on.

[crutchcorn]

@amirburbea I actually struggle a lot when it comes to allowSyntheticDefaultImports and such - so I admittedly did not know that! :) I'm much better at the actual typings themselves 😅 I'd love a review from you on the README.md changes I just made and would love if you'd be able to test both to confirm the validity of my statements.

[crutchcorn]

This might mean that the comment here: #785 (comment)

Is not accurate. Again, I'll defer to @amirburbea as I'm unfamiliar with the different import techniques w/ the config settings

[crutchcorn]

This is confirmed working by some folks testing from the issue. @hipstersmoothie let's go ahead and merge this, cut a release, then I'll open up a new issue for the problems with the 3.1 typings, then make a PR to fix that

[hipstersmoothie]

🚀 PR was released in v0.8.3 🚀