search

agilgur5 / react-signature-canvas

A React wrapper component around signature_pad (in < 150 LoC). Unopinionated and heavily updated fork of react-signature-pad
https://agilgur5.github.io/react-signature-canvas/
Other
547 stars 119 forks source link

add typescript definitions #25

Closed chillitom closed 4 years ago

chillitom commented 6 years ago

should close #21

agilgur5 commented 5 years ago

Thanks for the (much requested) contribution @chillitom ! Took me a while to finally find the time to give this a proper review as there's a lot to consider here

Unfortunately, I'm not sure this should be merged. Aside from my comments in https://github.com/agilgur5/react-signature-canvas/issues/21#issuecomment-407587231, which still stand, I also did some research since then and found that the TypeScript team themselves has explicitly said to publish types to DefinitelyTyped (@types) if the source code is not TS in https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html (c.f. https://github.com/inikulin/parse5/issues/235#issuecomment-362423707 and https://github.com/inikulin/parse5/issues/235#issuecomment-362446431). This seems to lend a strong addition to my argument that "third-party typing makes more sense at this time".

I attempted to auto-generate the typings with lyft/react-javascript-to-typescript-transform to circumvent this argument, but not only is there quite a bit more work to do from this transform, it actually doesn't generate types, it transforms the entire JS into TS (with different formatting too). Its types are also not as specific as the ones from the signature_pad TS beta (which should always remain the same, if not just imported)

I added some comments & commits to the PR which made me a bit more familiar with TS, but I'm still not sure this should be merged (and my uncertainty in the comments certainly doesn't help 😅). Open to thoughts and counter-arguments

benny-medflyt commented 5 years ago

I believe that this PR is missing the style prop, which is valid and will be passed along to the underlying signature implementation

agilgur5 commented 5 years ago

@benny-medflyt style is not an option of signature_pad and nor is it a direct prop of react-signature-canvas. You can only pass style through the prop canvasProps, which is already typed here.

remiroyc commented 4 years ago

I tried to use your lib @agilgur5, but my compilation failed because it could not find a declaration file for module 'react-signature-canvas'.

I think the good way is to publish "@types/react-signature-canvas" on npm. For that, you can propose your type definition here: https://github.com/DefinitelyTyped/DefinitelyTyped#create-a-new-package

agilgur5 commented 4 years ago

@remiroyc yes, as you can see, this PR is incomplete and has not been merged (and #21 is still open), therefore there are no types for this pure JS library. And as you can see above, I pretty much recommended the author of this PR submit these types to DefinitelyTyped (though there are still improvements to be made, some of which I made myself here).

As I wrote in https://github.com/agilgur5/react-signature-canvas/issues/21#issuecomment-407587231, at the time this PR was written, I had never used TypeScript, so I could not reasonably maintain the typings myself, and no one stepped up to maintain them here or at DefinitelyTyped (including the author of this PR, who has not responded to comments).

I've now written a bit of TypeScript and actually do have a ts branch locally, and expect to have a TS re-write out in v1.1.0 before the new year. Now that there's 100% test coverage, that should also give us some confidence that the types are correct before publishing (might add some more tests too).

Was actually going to comment about this in #21 sometime soon, but guess I'll summarize some here. The big blockers are rewriting the 3.5 year old build system used here (it is still on webpack 1) and rewriting the example once the build system is changed (and possibly having JS and TS versions of it).

I was planning on using tsdx to make swapping the build system easier, but https://github.com/jaredpalmer/tsdx/issues/165 is a big blocker for this library that uses default exports. Fortunately, I wrote a PR to solve that recently at https://github.com/jaredpalmer/tsdx/pull/327, but I have no idea when that will be merged or what the hold on it is. I recently created a fork with my changes included and so am planning on using that similar to what I did in https://github.com/agilgur5/mst-persist/pull/24.

For the example, I'm planning on just rewriting it with CRA, similar to the cra-example branch. Just need to figure out what to do about JS vs TS examples.

alkafaiz commented 4 years ago

Hi there, can you please merge this PR. I'm having trouble importing this module into my project due to this issue

agilgur5 commented 4 years ago

Please read the discussion, review, comments, related issues, etc before making comments. Locking this due to multiple comments that have not.

agilgur5 commented 4 years ago

Closing this as @types/react-signature-canvas has been released thanks to @ksocha in https://github.com/agilgur5/react-signature-canvas/issues/21#issuecomment-570812385 .

Per the above comments, this is the solution recommended by the TS team for third-party typings that are not generated from TS source code.

A TS rewrite is still in-progress and that will eventually replace the unofficial third-party typings with official, internal typings, planned for the 1.1.x release

agilgur5 commented 2 years ago

Noting here that the internal re-write, #42, has now been merged. Planning to release as 1.1.0-alpha.1 soon