benwinding / quill-image-compress

A Quill rich text editor Module which compresses images uploaded to the editor
https://benwinding.github.io/quill-image-compress/src/demo.html
MIT License
123 stars 30 forks source link

no types #30

Closed Johnrobmiller closed 2 years ago

Johnrobmiller commented 2 years ago

There are no ts types for this library.

benwinding commented 2 years ago

This is true. It's currently a JS lib, would you like me to add types?

Johnrobmiller commented 2 years ago

This is true. It's currently a JS lib, would you like me to add types?

haha it's up to you. I just added \\ @ts-expect-error above the import and it worked fine without types.

Ideally all libraries should be typed, but it's not a big deal to be honest.

Johnrobmiller commented 2 years ago

🎉

chpio commented 1 year ago

This is not fixed. Would need a "types": "types/index.d.ts", line in the package.json.

The usage seem to have changed as well (breaking change! and needs an update in the readme):

import ReactQuill, {Quill as OriginalQuill} from 'react-quill';
import QuillImageCompress from 'quill-image-compress';

OriginalQuill.register('modules/imageCompressor', QuillImageCompress);

const MODULES: Record<string, unknown> = {
  imageCompressor: {
    quality: 0.7,
    maxWidth: 1000,
    maxHeight: 1000,
    imageType: 'image/jpeg',
  },
};

(imageCompressor instead of imageCompress. imho the old name would be more fitting as it matches the name of the package)

is #27 related to this change?

also thank you for this lib and the ts rewrite

benwinding commented 1 year ago

This is not fixed. Would need a "types": "types/index.d.ts", line in the package.json.

Good catch! Thanks for reporting! 🙏

The usage seem to have changed as well (breaking change! and needs an update in the readme):

This isn't a breaking change at all, as the Quill.register() command is what signals the options object key.

For example, this here works too:

OriginalQuill.register('modules/imageCompressor123', QuillImageCompress);
const MODULES: Record<string, unknown> = {
  imageCompressor123: {
    ....
  },
};

Will change existing examples to all be imageCompress for clarity though 👍

chpio commented 1 year ago

yeah, you're right. My mistake. How does quill know what property it does need to use as the constructor?

benwinding commented 1 year ago

How does quill know what property it does need to use as the constructor?

Quill internally assigns the key to the class provided, using Quill.register('module/key', MyClass) so it doesn't matter what the class is called, it's referenced by the key. (also in JS classes can be passed around like functions)

This specific example is also using the "default export" of the JS module. Which can be named anything, as it's not a "named export"

Which means this will work too!

import MyQuillImageCompress123 from 'quill-image-compress';

OriginalQuill.register('modules/imageCompressor123', MyQuillImageCompress123);

const MODULES: Record<string, unknown> = {
  imageCompressor123: {
    ....
  },
};

JavaScript is a fun language! :rage1:

chpio commented 1 year ago

This specific example is also using the "default export" of the JS module.

yeah, but you do not do that

https://github.com/benwinding/quill-image-compress/blob/7f1ebda29594934c8b0579a5cd6d4d4101728019/src/index.ts#L4-L5

edit: nevermind

https://github.com/benwinding/quill-image-compress/blob/7f1ebda29594934c8b0579a5cd6d4d4101728019/package.json#L5

And i somehow misinterpreted

import QuillImageCompress from 'quill-image-compress';

for

import * as QuillImageCompress from 'quill-image-compress';
chpio commented 1 year ago

see my edit from the last post. that implies that my types line was wrong, and should have been "types": "types/quill.imageCompressor.d.ts"

ps: i wouldn't build the files minified if i were you, makes debugging harder and you have to minify your end product anyway

benwinding commented 1 year ago

Thanks again!! 😅 Yeah I just needed to export the default module in the index, fixed here 4bfbefe Should be deployed as version 1.2.29