[react-signature-canvas] Add definitions

[ksocha]

It's my first added definition file. Please, be gentle 🙏
https://github.com/agilgur5/react-signature-canvas

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).
• The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
• If this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.
• Create it with dts-gen --dt, not by basing it on an existing project.
• Represents shape of module/library correctly
• tslint.json should be present and it shouldn't have any additional or disabling of rules. Just content as { "extends": "dtslint/dt.json" }. If for reason the some rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.
• tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

[typescript-bot]

@ksocha Thank you for submitting this PR!

Because this is a new definition, a DefinitelyTyped maintainer will be reviewing this PR in the next few days once the Travis CI build passes.

In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day!

[typescript-bot]

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

These typings are for a package that doesn’t yet exist on master, so I don’t have anything to compare against yet! In the future, I’ll be able to compare PRs to react-signature-canvas with its source on master.

Comparison details 📊

Batch compilation
Type count	22280
Assignability cache size	53080
Language service measurements
Samples taken	83
Identifiers in tests	83
getCompletionsAtPosition
Mean duration (ms)	582.3
Mean CV	10.9%
Worst duration (ms)	762.8
Worst identifier	velocityFilterWeight
getQuickInfoAtPosition
Mean duration (ms)	572.1
Mean CV	8.9%
Worst duration (ms)	708.8
Worst identifier	onBegin
System information
Node version	v12.13.1
CPU count	2
CPU speed	2.294 GHz
CPU model	Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz
CPU Architecture	x64
Memory	6.8 GiB
Platform	linux
Release	4.15.0-1064-azure

[agilgur5]

Hi there, I'm the creator/maintainer of the react-signature-canvas package.

Thanks for submitting these types and tests @ksocha !! 🙂

The main issue I see here is that this relies on types from signature_pad v3, whereas react-signature-canvas uses v2 under-the-hood. v3 has been in beta for a while and has had some bugs (it seems to be unmaintained).
As this is a separate package that should only be used at build-time, it might be able to duplicate the dependency on signature_pad without increasing bundle size or causing dependency mismatch errors, but I'm not sure. Idk how the TS compiler would even handle that. There's no package.json here either, I'm not sure how that works for DefinitelyTyped (haven't read up on the contributing guidelines yet).
But even if it could be duplicated without issues, there could be type inconsistencies if v3 changes them without a major release (could pin the version here to prevent that though)

---

The other thing is that I'm working on rewriting the package in TS already. I've been contributing heavily to tsdx the past month, fixing bugs and adding features so I could use it in the rest of the packages I maintain, including react-signature-canvas, so that put a bit of a hold on the rewrite (v0.12 was released recently).
Have been looking to push an alpha soon, but something popped up and I'm out of the country for most of January. If this passes review prior to then, I'm totally ok with this being released as a stop-gap solution (gives me more time to polish a release too!!), but just noting that this may cause some churn for users.
Also not sure what the process is for adding deprecation notices to DT packages is (i.e. when TS is supported internally, these should be deprecated).

[agilgur5]

I think it would be better as ISignatureCanvasProps, but maybe even better to be named as IReactSignatureCanvasProps?

[ksocha]

I'm against this as other type definitions rarely use this convention of prepending I to the interface name. I personally think it brings no value and I've even seen linter rules to prevent naming interfaces like this.

I will change the name to ReactSignatureCanvasProps though.

[agilgur5]

Is HTMLCanvasElement the right generic type? I thought it should be canvasProps<T>?: React.CanvasHTMLAttributes<T>, but I'm not 100% sure. If you could provide an explanation, would be helpful to understand better 🙏 .

See same discussion in agilgur5/react-signature-canvas#25 (comment)

[ksocha]

I believe this is the only proper way to type the attributes of a canvas element. This is how it's used in React itself:

canvas: React.DetailedHTMLProps<React.CanvasHTMLAttributes<HTMLCanvasElement>, HTMLCanvasElement>;

[agilgur5]

Huh, confusing why it would be a generic then 🤔 I guess it passes type-checking in the tests, so that does mean it works o.o.

[ksocha]

I think it's generic because of consistency. In the React's type definitions, CanvasHTMLAttributes is defined as:

interface CanvasHTMLAttributes<T> extends HTMLAttributes<T> {
    height?: number | string;
    width?: number | string;
}

IntrinsicElements interface which defines all html elements looks like:

[...]
button: React.DetailedHTMLProps<React.ButtonHTMLAttributes<HTMLButtonElement>, HTMLButtonElement>;
canvas: React.DetailedHTMLProps<React.CanvasHTMLAttributes<HTMLCanvasElement>, HTMLCanvasElement>;
caption: React.DetailedHTMLProps<React.HTMLAttributes<HTMLElement>, HTMLElement>;
[...]

Generic type passed to HTMLAttributes is used mostly (if not only) in event handlers definitions, e.g.: onFocus?: FocusEventHandler<T>;

[ksocha]

Hi there, I'm the creator/maintainer of the react-signature-canvas package.

Hi @agilgur5 !

The main issue I see here is that this relies on types from signature_pad v3, whereas react-signature-canvas uses v2 under-the-hood.

Type definitions for signature_pad are for v2. There is a way to pin down a specific version (by using paths as described here) but we cannot do this now, because there are no definitions for v3.

The other thing is that I'm working on rewriting the package in TS

As this is quite a big change, have you considered bumping a major version number of the lib? This way, people who would use v1 could still use definitions from DT, while all who would use v2 will get definition file generated by the lib itself.

[agilgur5]

Type definitions for signature_pad are for v2. There is a way to pin down a specific version (by using paths as described here) but we cannot do this now, because there are no definitions for v3.

Ah, so this is using types from @types/signature_pad, which types v2? Should probably use those in the rewrite as well, didn't know those existed.
v3 is written in TS and ships types internally (no need for DT third-party typings).

As this is quite a big change, have you considered bumping a major version number of the lib? This way, people who would use v1 could still use definitions from DT, while all who would use v2 will get definition file generated by the lib itself.

Yes, I have considered that:

• I plan to have at least one alpha release to make sure the transpiled output hasn't changed in breaking ways (similar to the v1 alpha), but otherwise was planning on releasing it as v1.1.0.

  • It is an internal restructuring that just adds new outputs like types (and a few more outputs that come with TSDX). Per SemVer, that fits relatively neatly into a minor version and I have done the same in other libraries I maintain. It makes for a seamless upgrade for those who want types as well as future patches & new features.
  • Also, the addition of 100% test coverage was a much bigger internal change IMO, but that made 0 external changes (aside from some docs) and so was released as a patch.

• Bumping major versions would cause version churn and has significant potential to leave users behind. I don't think that makes sense to do unnecessarily or without significant cause.

  • Notably, v1.x has existed for nearly 2 years now, so leaving users behind is a very serious concern.

• I also don't plan on backporting patches to v1.0.x as they'd have to be in JS (vs. TS) and a minor upgrade is not breaking -- that is far more complex if it were a major.

  • On top of that, if it were a major, it would also mean these third-party types and the internal types should be maintained during patches.
    • I did not make a PR to DT myself and do not plan on maintaining these myself either; I plan on maintaining the internal types. If it were a major change simply because of this, then @types/react-signature-canvas effectively becomes official, and implicitly means I'd have to support and maintain it.
      • Whether I'd want to or not, users would file issues against react-signature-canvas and would come to me. People have literally reached out to me via Twitter, Email, LinkedIn, etc for support for libraries related to mine that I didn't make or publish, so I'd prefer not to create that problem myself with this library if I don't have to.

• Churning all users of react-signature-canvas is a far worse trade-off than just churning users of @types/react-signature-canvas, especially given that it is a stop-gap, temporary solution.

Suffice to say, there's quite a lot of details to consider when making breaking/major changes, and most of those seem like very big cons to me.

But even without getting into those details & trade-offs, the release of third-party typings on DT during an internal TS rewrite shouldn't significantly impact the release of the internal rewrite. They are third-party, and, on top of that, that would suggest that timing, instead of content, is the biggest factor in deciding to do a major.

[agilgur5]

Some frequently used canvasProps are width, height, and style or className (CSS backgroundColor is a topic that pops up occasionally) -- would probably be good to use those instead

[ksocha]

• I plan to have at least one alpha release to make sure the transpiled output hasn't changed in breaking ways (similar to the v1 alpha), but otherwise was planning on releasing it as v1.1.0.

It will work fine as well. For people still using v1.0.x (or those who do not want to update to v1.1.x or newer), DT type definitions will be a way to go.
If someone updates to >= v1.1.x, all he has to do is to remove @types/react-signature-canvas from the dependencies.

[sandersn]

Chiming in as a DT maintainer: DT has a npm task that deprecates an @types package. You can deprecate anything that has a newer version of the source package -- doesn't have to be a major version. Of course you can keep the @types package around if you believe the 1.0.x types will continue to need fixes.

Also: when resolving import 'react-signature-canvas', the compiler always looks in the source package for types before looking at @types. So there's no harm in having @types/react-signature-canvas installed at the same time as a react-signature-canvas with types.

[typescript-bot]

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

[armanio123]

@agilgur5 @ksocha Is there anything pending for merging or this is approved?

[ksocha]

@armanio123 From my point of view all work is done.