Data given to `fromData` is mutated by the component about react-signature-canvas HOT 6 CLOSED

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 🙂

from react-signature-canvas.

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 😄), 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 😉)

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.

from react-signature-canvas.

Merged! Well done!

from react-signature-canvas.

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 szimek/signature_pad#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 },
//   ]
// ]

from react-signature-canvas.

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).

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?")

from react-signature-canvas.

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)

from react-signature-canvas.