TypeScript import won't work #785
Comments
|
Seriously considering moving the definition to definitely typed. There have been countless issues around managing the types in this project. I love typescript but I'm starting to believe that if a library isn't shipped in TS it shouldn't try to ship .d.ts files too. |
|
If we do keep theme we definitely need some type testing @crutchcorn |
|
Blech, I tried to test myself but musta dropped the ball somewhere - okay. I'll see if I can write some tests tomorrow and get a PR to fix this I totally get if we want to move the types to their own repo. I'm willing to maintain these types for this repo, and I feel that when typings are moved out of a project they're maintained to a lesser degree and they suffer as a result, but that's my initial thoughts @cancerberoSgx, regarding the type complexity - I agree that they're just complex but they're forced to be so if the plugin system is able to be properly and strictly typed. While it's unfortunate that such complex typings are required, I sincerely did my very best to document things as I could and make the typings as verbose and clear as possible (I'd love feedback on how they might be made more straightforward without removing the plugin functionality - it's within huge possibilities I missed something obvious) |
|
Working on it more today and I think I have it fixed That said, I am writing dts-lint tests (the same in DefinitelyTyped) to try to ensure to the best of my abilities it will work |
|
I think that should help with future PRs for types too. Thanks for your work! It's my fault for the plugin system 😅 If any fingers should be pointed for needless complexity it's at me lol. |
|
For sure. Huge apologies for introducing a PR without doing more thorough testing (it turns out my IDE was doing some weird things to fix the mistaken type errors and I just assumed it worked) - I'm trying to prioritize getting a fix out today for it since the version was deployed I think the plugin system is great - it's never easy to introduce one and it seems to work really well |
|
Issue has been resolved in PR #786 and typing tests were added to confirm functionality. Again, huge apologies |
|
I spent 1 hour trying to figure why the hell I was not able to type at all my generic mixins until I found this issue. |
|
@crutchcorn Aja, I'm not experienced with jimp plugins, but in other projects what they do is pass the responsibility declaring plugin types to plugin authors. If you delegate this to community definetely typed then they will have the same problem. This is what project jest does for example (my idea BTW ) : https://jestjs.io/docs/en/expect#expectextendmatchers Basically If a plugin adds a new member to an existing interface or adds new types or modifications to existing types thet are responsible of declaring those modifications / new signatures by themself. Jimp project just need to be sure to expose the types that are susceptible of being modifiable by plugin authors. If you need a hand with this I will be glad to help, but need your confirmation so I start familiarizing with your plugin's API and so on. I strongly advice that, if you are interesting in supporting TypeScript definitions maintaining them here will add value and quality to this project for sure!- -will you let me know if you need a hand with it ? THanks |
|
@cancerberoSgx It's not quite that easy. The plugin authors in this case is us: https://github.com/oliver-moran/jimp/tree/master/packages/plugins The other issue is that we're enabling composing of these packages via function ala: https://github.com/oliver-moran/jimp/tree/master/packages/custom Using interface merging as you're suggesting (and as jest uses) only extends the initial type without doing any strictness testing to see if the This PR should have the typings fixed #786 and introduces type testing. If you're wanting to contribute to these typings, a code review of them would be phenominal (I'd love if we're able to maintain the type scrictness for |
Ajá, again, totally new with these APIs, and I assume you cannot solve the problem with mixings or modifying ambient modules. I will try to take a look to your code, but just in case (and this is personal opinion) in these cases I always have a small TS project between my test assets as part of my CI just to make sure the compilation doesn't fail (check the readme example works). Is not important to reflect there that plugins typings works, but just that the simple TS example in the readme don't fail. Also another feedback - don't describe three different import methods in the readme - it's too verbose. In general you just want to describe the import statement both JavaScript and TypeScript users will use. TypeScript users knows how to configure their project to make it work - and in general when developing in the web they always use Last feedback: avoid default exports or assignment exports to the user - so they just can Thanks again! Keep it up |
|
Said PR introduces fairly complete typing tests (that tests against 2.8 all the way to 3.6 using We're also shipping a version of the typings for 2.8 seperate from the ones for 3.1+ (as those are built on top of the plugin system and require some functionality that older versions of TS dont have). As for the documentation of various import methods - I think that's a seperate set of concerns. I don't know if that's related to the work being done on typings here Also, regarding default exports - this is a two-fold problem:
|
|
@cancerberoSgx Could you verify that v0.8.1 fixes you issue? |
|
Doesn't work for me, when I do
or
with Jimp 0.8.1 from npmjs, all I get is
allowSyntheticDefaultImports is true (setting it to false seems to result in the same thing, but also a bunch more unrelated errors) (Using TS 3.6.3 + Node 10.16.3; Last working Jimp version: 0.6.4) |
|
This gave me a heart attack until I looked at it really quick this morning - good news is this is a trivial fix. When doing testing for TS, in order to avoid issues with I'll have a PR open that should fix this, should be a really small lift |
|
@patheticcockroach downgrading to 0.7.0 again works for now. Hopefully 0.8.2 will fix us. |
|
#792 fixes the Thank you all for the patience on this issue. I know the 0.8 release has been rocky but I think the types are in a much much more maintainable place now (both with organization and typing tests) and I'm hoping that once this PR is merged things can move from here without too much of a hitch. |
|
If anyone wants to test the PR for #792 the package version (that I confirmed fixed the issue in my testing at least) released as a canary is @patheticcockroach would you be able to test against this version really quick? |
|
It worked for me
…On Thu, Sep 12, 2019, 11:27 AM Corbin Crutchley ***@***.***> wrote:
If anyone wants to test the PR for #792
<#792> the package version (that
I confirmed fixed the issue in my testing at least) released as a canary is
0.8.2-canary.792.283.0.
@patheticcockroach <https://github.com/patheticcockroach> would you be
able to test against this version really quick?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#785>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAQGPCQNZR5YQIF5GBSZHQ3QJJNWPANCNFSM4IUSDLGA>
.
|
|
I'm now getting a bunch of those:
In code like:
|
|
@patheticcockroach what version of TS are you using and how are you importing? (I think this may be a really simply fix that I can add to that PR, thank you for testing) |
|
This is the tsconfig I am using and it fails to compile actually - I am not sure why but it seemed to work earlier. {
"compileOnSave": false,
"compilerOptions": {
"allowJs": false,
"allowSyntheticDefaultImports": true,
"allowUnreachableCode": false,
"allowUnusedLabels": false,
"alwaysStrict": true,
"declaration": true,
"declarationMap": true,
"disableSizeLimit": true,
"emitBOM": false,
"emitDeclarationOnly": false,
"emitDecoratorMetadata": false,
"esModuleInterop": true,
"experimentalDecorators": true,
"forceConsistentCasingInFileNames": true,
"importHelpers": true,
"incremental": true,
"isolatedModules": false,
"lib": ["dom", "es2015", "es2016", "es2017.object"],
"module": "esnext",
"moduleResolution": "node",
"newLine": "LF",
"noEmitOnError": true,
"noErrorTruncation": true,
"noFallthroughCasesInSwitch": true,
"noImplicitAny": true,
"noImplicitReturns": true,
"noImplicitThis": true,
"noStrictGenericChecks": false,
"noUnusedLocals": true,
"preserveConstEnums": false,
"preserveSymlinks": true,
"pretty": true,
"removeComments": false,
"resolveJsonModule": true,
"sourceMap": true,
"strict": true,
"strictBindCallApply": true,
"strictFunctionTypes": true,
"strictNullChecks": true,
"strictPropertyInitialization": true,
"stripInternal": true,
"suppressExcessPropertyErrors": false,
"suppressImplicitAnyIndexErrors": false,
"target": "esnext"
},
"exclude": ["./node_modules/"]
}The types load but import Jimp from 'jimp';
export default async function() {
const font = await Jimp.loadFont(Jimp.FONT_SANS_16_WHITE); //succeeds
const output = new Jimp(400,400,'#ffffff'); // error - type is not constructable |
|
@crutchcorn you might need to add something like interface Constructable<T> {
new (): T;
} |
|
I'm using TS 3.6.3 my tsconfig is: {
"compileOnSave": true,
"compilerOptions": {
"target": "es5",
"noImplicitAny" : true,
"strictPropertyInitialization": false,
"strictNullChecks": true,
"esModuleInterop": true,
"lib": [
"es5",
"es2015.promise",
"esnext.asynciterable"
],
"skipLibCheck": false,
"alwaysStrict": true,
"removeComments": true,
"typeRoots": [
"node_modules/@types"
],
"moduleResolution": "node",
"baseUrl": ".",
"paths": {
"*": [
"*"
]
}
},
"exclude": [
"node_modules"
]
}And then in my ts files I do import Jimp from 'jimp'; |
|
@hipstersmoothie please revert 0.8 the typings need significantly more thoughts placed into them (I earnestly had forgotten about the constructor and it threw significant amount more loops then expected) and I've had 5 nights of maybe 3 hours of sleep each trying to work on this (and tons of stuff going on outside of this that I need to take care of) I'll loop back in a week (I don't think the work I've done is unsalvagable - just requires more time then I have right now, I'm feeling burnout REALLY bad right now) - but I'm officially going AFK on this issue until I'm of better state of mind, more well rested, and have more of a schedule to be able to take these considerations on |
|
PR #792 was updated to remove the new typings for 3.1 (from being detected by TS, without actually removing the code). @hipstersmoothie if you merge, there will still be a value add to TS users for 0.8 due to the new plugins being typed, but keeps the old type interface only exposed (so even TS3.1 users will use the old typings). @patheticcockroach please test |
|
I downloaded the above version. I'm sorry but it still failed for 2 reasons.
The problem lies here (line 414) declare const JimpInst: Jimp;
export default JimpInst;This is an export of a single instance of the class, so typescript can't simply new it up. |
|
@patheticcockroach
of if you really need to include the whole current dir at least : include: ["./*.ts"] os something that don't force you to exclude node_modules.... ? |
|
My use of 3.6.3 and the tsconfig above (#785 (comment)) works just fine with 0.7.0 @patheticcockroach - maybe you want to try that? It won't solve 0.8+, but it will get you closer to the latest version |
|
I tried the tsconfig from #785 (comment), I was still getting (among lots of additional errors):
I believe something happened in this commit 7dff287 private doStuff(image1: typeof Jimp, image2: typeof Jimp): typeof Jimp {
.... stuff
}the errors eventually all disappear, and version 0.7.0 "works". But this really looks dirty |
import jimp_ from 'jimp' will save you for ugly refactor too |
|
I feel like at this point there must be something that gets it back to that behavior. If either of you want to take a stab at it I would be grateful ❤️ |
|
When trying to build my app I get these errors all over the place using
None of that can be seen on version 0.6.8 though. |
|
Sorry to say that 0.8.2 is also no bueno. It says can not find types for @jimp/core |
|
Alright y'all. I know I've been gone for a while now, but I have a new version for everyone to test. This includes typing tests for things like: import Jimp from 'jimp'
const jimpInst: Jimp = new Jimp()And more. Please be patient and understand if this version does not work (I am still putting a fair amount of time into this outside of my full-time job), but I'd love some feedback on: Please ReadNow, I do want to make a note: Both of these import methods were broken as-of 0.6.4 (hense why that was the last version that worked for you, ), and unless we remove all other named exports, there is nothing that can be done there. The good news is: This should not impact the ability for your code to run. When looking at the built version of the code, you can clearly see: var _default = (0, _custom.default)({
types: [_types.default],
plugins: [_plugins.default]
});
exports.default = _default;
module.exports = exports.default;That is to say (for those of us who don't speak ES6 Babel nonesense): The export is duplicated to have both ES6 and commonJS exports. It will work regardless of how you import it in TS (but the typings, I will remind, will not). This is all a long TLDR that's to say: Before you test this version - please make sure to change all of your imports to use the default |
|
FWIW, the canary release I mentioned works for our use-case in https://github.com/akfish/node-vibrant/blob/develop/packages/vibrant-image-node/src/index.ts#L8 As well as the suggested change to add in |
|
It's working quite better, I only have a couple of errors left. The first relevant parts of code look like this: import Jimp from 'jimp';
let finalImage: Jimp = new Jimp(width, height, 0x00000000);
return finalImage.resize(newWidth, newHeight);and I get this error: The second one looks like: import Jimp from 'jimp';
this.baseImage = await Jimp.read('filename');
this.finalImage = this.baseImage.clone()
.resize(1, 1)
.setPixelColor(0x00000000, 0, 0)
.resize(width, height);and I get this error: No other errors, so this is clearly going in the right direction 👍 |
|
@patheticcockroach I am seeing the However I don't get this error you have That again sounds like For example, I use Jimp in a webpack plugin to create a splash screen for my electron app, and this canary build works for me with my code and tsconfig below. import webpack from 'webpack';
import Jimp from 'jimp';
import path from 'path';
export class SplashPlugin {
constructor(private readonly options: SplashPlugin.Options) {}
apply({ hooks }: webpack.Compiler) {
const { name, version, env } = this.options;
const text = `${name} v${version}${env !== 'PROD' ? ` (${env})` : ''}`;
hooks.emit.tapPromise(SplashPlugin.name, async compilation => {
const font = await Jimp.loadFont(Jimp.FONT_SANS_16_WHITE);
const source = await Jimp.read(path.resolve(__dirname, '../logo.png'));
const output = new Jimp(450, 250, '#2b384b')
.opaque()
.composite(source, 0, 0)
.print(
font,
0,
200,
{
text,
alignmentX: Jimp.HORIZONTAL_ALIGN_CENTER,
alignmentY: Jimp.VERTICAL_ALIGN_MIDDLE
},
450,
50
)
const buffer = await output.getBufferAsync(Jimp.MIME_PNG);
compilation.assets['splash.png'] = {
source: () => buffer,
size: () => Buffer.byteLength(buffer)
};
});
}
}
export namespace SplashPlugin {
export interface Options {
name: string;
version: string;
env: 'DEV' | 'PROD' | 'UAT';
}
}{
"compileOnSave": false,
"compilerOptions": {
"allowJs": false,
"allowSyntheticDefaultImports": true,
"allowUnreachableCode": false,
"allowUnusedLabels": false,
"alwaysStrict": true,
"declaration": false,
"disableSizeLimit": true,
"emitBOM": false,
"emitDeclarationOnly": false,
"emitDecoratorMetadata": false,
"esModuleInterop": true,
"experimentalDecorators": true,
"forceConsistentCasingInFileNames": true,
"importHelpers": true,
"incremental": true,
"isolatedModules": false,
"lib": ["dom", "es2015", "es2016", "es2017.object"],
"module": "commonjs",
"moduleResolution": "node",
"newLine": "LF",
"noEmitOnError": true,
"noFallthroughCasesInSwitch": true,
"noImplicitAny": true,
"noImplicitReturns": true,
"noImplicitThis": true,
"noStrictGenericChecks": false,
"noUnusedLocals": true,
"outDir": "./lib",
"preserveConstEnums": false,
"preserveSymlinks": true,
"pretty": true,
"removeComments": false,
"rootDir": "./src",
"sourceMap": true,
"strict": true,
"strictBindCallApply": true,
"strictFunctionTypes": true,
"strictNullChecks": true,
"strictPropertyInitialization": true,
"stripInternal": true,
"suppressExcessPropertyErrors": false,
"suppressImplicitAnyIndexErrors": false,
"target": "esnext"
},
"exclude": ["./node_modules/"]
} |
|
@patheticcockroach - a temporary workaround to the resize method could simply be module augmentation. declare module '@jimp/core'{
class Jimp {
resize(x:number,y:number):this;
}
}
import Jimp from 'jimp';
function doSomething() {
const output = new Jimp(450, 250, '#2b384b').resize(10, 10).opaque()
... |
|
allowSyntheticDefaultImports was already set to true, and removing the explicit assignment didn't change things. However, adding this: declare module '@jimp/core'{
class Jimp {
public resize(x: number, y: number): this;
}
}as you suggested, fixed both errors |
|
I am able to recreate the issue described by @patheticcockroach. It seems the |
|
The resize method definition simply seems to have been omitted in |
|
The resize method shouldn't be on Where |
|
@cancerberoSgx I'm wondering if we're wanting to move forward with the PR in #792, close this ticket, then make a new ticket for the concern with |
|
Actually, @patheticcockroach can you test this one last time against If this commit works, the typings WILL fully work, but will not utilize the new plugin system. |
It works! :) |
|
@patheticcockroach when you get a chance, it'd be great if you can test against #798 (with the same import restrictions of 0.8.3 [which was the last version you said worked - it got published]). If that works, then the 3.1 TS migration should be complete. I am so sorry this was such a frustrating lengthy process. The type tests put in place should DRASTICALLY reduce the potential for this type of thing to happen again |
|
This issue should be solved as-of 0.8.4. I'd love to see it closed. Please keep in mind that, per the updated README, |
|
This doesn't seem to be fixed as far as I can see.
the first time I try to use Jimp:
I'm using version 0.8.4. |
|
If you need to use that old = require syntax, you don't have
esModuleInterop/allowSyntheticDefaultImports both on.
…On Sun, Sep 22, 2019, 7:13 AM William Warby ***@***.***> wrote:
This doesn't seem to be fixed as far as I can see.
const Jimp = require('jimp'); works fine, but if I use import Jimp from
'jimp'; I see
Cannot read property 'read' of undefined
the first time I try to use Jimp:
const image = (await Jimp.read(buffer));
I'm using version 0.8.4.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#785>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAQGPCRL4LAKQ5RZZSOEGY3QK5HMZANCNFSM4IUSDLGA>
.
|
|
Urgh. You're right. But when I turn both those on, about 10 other imports in my project break. |
|
@crutchcorn New version is working OK for me but I leave in your hands closing this issue. BTW I don't need to add any special configuration in tsconfig.json . THanks! |
|
@cancerberoSgx I am unable to close this. I'd love if you would be able to |
None of the three methods detailed in the readme worked for me when importing gimp on my TypeScript project when upgrading to [email protected]
I couldn't even access the right type so at least I can force a cast to require('gimp'). The error I'm seeing in the suggested methods is that the type resolved to never.
I'm using typeScript 3.6.2 , and node 12.8.1. I also tried with all combinations of
esModuleInteropandallowSyntheticDefaultImportsas the readme mention but without luck. I feel competent with TypeScript and I really think you have something wrong with your type declarations. It would be awesome if you could just export the constructor type - in my case I need to cast the Jimp class, as innew Jimp(). Also, just an opinion, these type declarations are unnecessary complex I think. Any tip is most welcome, thanks. This is my tsconfig.jsonThe text was updated successfully, but these errors were encountered: