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

Data given to `fromData` is mutated by the component #56

Closed Zwyx closed 2 years ago

Zwyx commented 4 years ago

To reproduce,

canvasRef.current.fromData(signaturePoints);

- draw a line on the canvas
- log the content of `signaturePoints`, it'll contain the line you just draw:
```typescript
console.log(signaturePoints);
// [
//   [
//     {x: 54.002685546875, y: 130.68206787109375, time: 1596180140938 },
//     {x: 87.002685546875, y: 86.68206787109375, time: 1596180141056 },
//     {x: 114.002685546875, y: 66.68206787109375, time: 1596180141072 },
//     {x: 130.002685546875, y: 60.68206787109375, time: 1596180141088 },
//     {x: 143.002685546875, y: 60.68206787109375, time: 1596180141104 },
//     {x: 150.002685546875, y: 61.68206787109375, time: 1596180141120 },
//   ],
//   [
//     {x: 132.002685546875, y: 110.68206787109375, time: 1596185355643 },
//     {x: 135.002685546875, y: 106.68206787109375, time: 1596185355725 },
//     {x: 146.002685546875, y: 98.68206787109375, time: 1596185355741 },
//     {x: 153.002685546875, y: 94.68206787109375, time: 1596185355757 },
//     {x: 157.002685546875, y: 91.68206787109375, time: 1596185355790 },
//     {x: 146.002685546875, y: 98.68206787109375, time: 1596185355741 },
//   ],
// ]

signaturePoints has been modified by the component.

A workaround to prevent this is to clone the data before passing it to fromData:

canvasRef.current.fromData(R.clone(signaturePoints));

R being Ramda. EDIT: or use [].concat(signaturePoints), or [...signaturePoints], which doesn't require a third-party library, to clone the data as agilgur5 recommends below.

agilgur5 commented 2 years ago

Marked this as upstream back when it was opened, but never quite responded. Thanks for providing a repro!

Upstream Bug, but might have performance implications

react-signature-canvas is a pretty simple wrapper around signature_pad, so this bug is upstream and not here. The one-liner code for fromData can be found on this line.

I dug a bit deeper upstream in signature_pad, and found this to be caused by this line: this._data = pointGroups;. This sets the internal pointer to be the same as the data passed into fromData.

Ideally, this should be copied over instead of using the ref so that mutations don't affect the arguments that were passed in. This is definitely an edge case though, as I've actually never come across a use-case for this. That being said, it's a simple change to fix this, so it could be contributed if there isn't a significant performance penalty to this (i.e. if the data is huge, copying could cause some lag).

Next Steps to fix

signature_pad recently had some large changes and has released v4.0.0 (v3 never made it past beta, so it was basically v2.5.2 -> v4.0.0). It's a breaking change, so it will likely require a similar breaking change in this package due to the shared API surface.

Part of that change was this PR https://github.com/szimek/signature_pad/pull/570 that changed that line a tiny bit, but did not quite fix this issue for all cases. However, a one-liner can be done to fix it for all cases, [].concat(pointGroups) instead of pointGroups. I'll try to make a PR for that soon in signature_pad and then I'll have to get the upgrade in as well to fix this.

Given the possible perf implications and the simple workaround available for this edge case, it's possible this won't be accepted though.

Workaround Available

canvasRef.current.fromData(R.clone(signaturePoints));

Thanks for providing a workaround with your issue! Per my above paragraph, this can be simplified with raw JS so as not to require use of a library like Rambda:

canvasRef.current.fromData([].concat(signaturePoints))

Some tests for this workaround

Also did some tests to make sure that there aren't further pointer issues deeper in the data structure with signature_pad's usage:

const signaturePoints = [
  [
    { x: 54.002685546875, y: 130.68206787109375, time: 1596180140938 },
    { x: 87.002685546875, y: 86.68206787109375, time: 1596180141056 },
    { x: 114.002685546875, y: 66.68206787109375, time: 1596180141072 },
    { x: 130.002685546875, y: 60.68206787109375, time: 1596180141088 },
    { x: 143.002685546875, y: 60.68206787109375, time: 1596180141104 },
    { x: 150.002685546875, y: 61.68206787109375, time: 1596180141120 },
  ],
]

const data = [].concat(signaturePoints)

// push is used in signature_pad: https://github.com/szimek/signature_pad/blob/878786ef6ef38bf317d1cf674b3455d39e98d270/src/signature_pad.ts#L299
data.push([
  {x: 132.002685546875, y: 110.68206787109375, time: 1596185355643 },
  {x: 135.002685546875, y: 106.68206787109375, time: 1596185355725 },
  {x: 146.002685546875, y: 98.68206787109375, time: 1596185355741 },
  {x: 153.002685546875, y: 94.68206787109375, time: 1596185355757 },
  {x: 157.002685546875, y: 91.68206787109375, time: 1596185355790 },
  {x: 146.002685546875, y: 98.68206787109375, time: 1596185355741 },
])

console.log(signaturePoints)
// [
//   [
//     {x: 54.002685546875, y: 130.68206787109375, time: 1596180140938 },
//     {x: 87.002685546875, y: 86.68206787109375, time: 1596180141056 },
//     {x: 114.002685546875, y: 66.68206787109375, time: 1596180141072 },
//     {x: 130.002685546875, y: 60.68206787109375, time: 1596180141088 },
//     {x: 143.002685546875, y: 60.68206787109375, time: 1596180141104 },
//     {x: 150.002685546875, y: 61.68206787109375, time: 1596180141120 },
//   ]
// ]
Zwyx commented 2 years ago

Nice investigation, thanks! Cloning the data can indeed have a performance impact. I believe that in this case, it's probably negligible (unless the signature is quite complexe :smile:), but it's definitely worth having it in mind.

Do you think that a PR changing this:

signaturePad.fromData(data, { clear: false });

to this 

signaturePad.fromData(data, { clear?: boolean, immutable?: boolean });

could have chances of being accepted? (It's not ideal like this, but it's a first step toward having it immutable by default for the next major release :wink:)

Thanks also for [].concat(signaturePoints), it's indeed better to not have to install a third-party library just for this. [...signaturePoints] is also possible.

agilgur5 commented 2 years ago

Cloning the data can indeed have a performance impact. I believe that in this case, it's probably negligible (unless the signature is quite complexe 😄), but it's definitely worth having it in mind.

Agreed, but figured I'd might as well test this and see to make sure. I did a quick test (see screenshot below) of cloning the full data of a signature and there was no noticeable lag on my end. Of course, this can vary per device (e.g. on a low-end mobile device vs. a beefy laptop), but given that fromData really shouldn't be used that often anyway, I don't see many use-cases where it may make a difference (the only one I could anticipate is reading then writing back on resizes, which could already induce lag in and of itself).

Screen Shot 2022-02-28 at 7 08 15 PM

could have chances of being accepted? (It's not ideal like this, but it's a first step toward having it immutable by default for the next major release 😉)

Now that I've tested the performance I'll try a PR with the clone. If not, I think that's a good alternative, though I would rename the parameter to something like cloneData instead to be more clear ("immutable" without context makes me think "immutable what?")

Zwyx commented 2 years ago

Hi @agilgur5 ! That's a really nice and simple PR. I'll keep an eye on it, and as soon as it's merged over there, I'll close this issue. Thank you for your work :slightly_smiling_face:

Zwyx commented 2 years ago

Merged! Well done!

agilgur5 commented 2 years ago

Linked this to #68 and a new tracking milestone as react-signature-canvas still has to update to signature_pad v4 (which is a non-trivial amount of work, per #68)