Bump signature_pad to v4 to fix undefined is not an object race condition

[M1ke]

This change resolves a race condition where the following message is logged:

TypeError: undefined is not an object (evaluating 'this._data[this._data.length-1].push')

Updating signature_pad from 2.x to 4.x includes a fix for szimek/signature_pad#329

[agilgur5]

doesn't seem to be iOS 15 specific, rewritten

an iOS 15 issue

per szimek/signature_pad#329, this seems to occur on multiple devices and browsers and doesn't seem to be related to iOS 15 specifically, so I've rewritten the description to be more generic

not sure that the upgrade fixes this either?

fixes szimek/signature_pad#329

I've rewritten this as well because fixes is a keyword in GitHub issues and it made this PR pop up in that issue as a "may fix", which is incorrect.

That being said, as szimek/signature_pad#329 (comment) points out, there actually hasn't been a confirmation that this is fixed in v4.
I did go ahead and investigate myself though, and if the root cause is the same race condition, then it should indeed have been fixed by szimek/signature_pad#481. But I'm not sure what the root cause is, this is just from a cursory reading of the related issues and PRs.

PR does not address breaking change

Per my in-line comment, this PR does not address a breaking change in signature_pad and as such cannot be merged. When making upgrade PRs, especially SemVer major ones, please make sure to check the changelogs and address them (and if no changes are required, stating that in your PR and why you believe that to be so can be very helpful to reviewers).

[agilgur5]

This upgrade is a breaking change, but this PR does not make any other changes outside of a version upgrade.

Per the changelog, they are incompatible versions, however, as the SemVer major tag change suggests.
The EventEmitter change is breaking and affects the onEnd and onBegin callbacks in this library/component. Those are currently in idiomatic React and don't work as smoothly/straightforward against addEventListener etc APIs. Will need to think of the optimal way to implement that, or require direct signature_pad usage or something, but at least one other internal change must be made in addition to the upgrade.

It may very likely require a major version change of react-signature-canvas if our API has to change in order to accommodate the EventEmitter upstream change. TBD on that

[M1ke]

Thanks for the thorough commentary. It seems we're a bit stuck in this case; the fix is trivial but if it requires a rewrite of the APIs and a major version bump in this library that becomes a lot of work.

Can I suggest an alternative proposal? This library currently relies on a version of signature_pad released over 4 years ago. The fix for the issue in that library which we need is 4 lines of code (excluding comments). I will attempt to get a PR onto the v2 branch of signature_pad and encourage a new minor version release there, meaning we can upgrade here whilst respecting semver.

If that's not possible because of the upstream package not wanting to put out a new minor for an old version, what would you think to forking that package with the fix applied to the v2 branch and using that as the dependency?

[UziTech]

I think a better option would be to update signature_pad to v4 and use the onBegin and onEnd props to add events to signature_pad

something like:

componentDidMount () {
    this._sigPad = new SignaturePad(this._canvas, this._excludeOurProps())
    if (this.props.onBegin) {
        this._sigPad.addEventListener("beginStroke", this.props.onBegin);
    }
    if (this.props.onEnd) {
        this._sigPad.addEventListener("endStroke", this.props.onEnd);
    }
    this._resizeCanvas()
    this.on()
  }

That way you get the most secure version without needing to maintain it yourselves and your API doesn't change. 🎉

You could also add onBeginUpdate and onAfterUpdate if you want. 😁👍

[M1ke]

That change looks pretty simple; do you think it's the only change required to upgrade to v4?

[UziTech]

That is the only breaking change to the inputs.

The other big breaking change is the structure of the data returned from toData. The main change their was a fix so each line segment has it's own properties for dotSize, minWidth, maxWidth (szimek/signature_pad#571) and the addition of pressure to the data points (szimek/signature_pad#566)

[agilgur5]

old signature_pad version

relies on a version of signature_pad released over 4 years ago

Several people have said things of this manner without any context, which I don't think is helpful to general discussion. signature_pad was not really maintained for a long while (hell, neither was this library, but both worked in most cases), so it's not as if there are 4 years' worth of daily changes there as this statement may suggest to some.
Similarly, "two major versions" is not altogether meaningful either as v3 never came out of beta (and was mostly just a TS rewrite) and a decision was made to just move to v4 instead.

The fix for the issue in that library which we need is 4 lines of code (excluding comments). I will attempt to get a PR onto the v2 branch of signature_pad and encourage a new minor version release there, meaning we can upgrade here whilst respecting semver.

That would certainly make things easier, but many maintainers are hesitant to patch old versions (with pros and cons to that approach). Appreciate that you made the PR!

react-signature-canvas also wouldn't need to upgrade what-so-ever in that case given that we use ^2.3.2 as a range. So users would just need to update their deps / package-lock.json / yarn.lock etc.

making v4 changes backward compatible

Thanks for your comments here @UziTech . Unfortunately, the suggested change is overly simplistic in a number of ways.

componentDidMount () {

Only adding the event listener in componentDidMount would not be a complete solution, given that the callbacks can be changed at any time. Event listeners would likely have to be added and removed within componentDidUpdate, which is where props are currently propagated to signature_pad. A simple add would not be enough as changes to the callbacks would then keep creating new event listeners ad infinitum instead of a 1-to-1 mapping.
On that note, onBegin and onEnd will also have to no longer be propagated to signature_pad as well.

It may be possible to do something similar to how resizes are currently handled and always attach a "wrapper" event listener instead. The "wrapper" function would effectively do nothing if there is no callback, but call the callback if there is one. There may or may not be some complications with this (e.g. this), would have to test a good bit.

Speaking of tests, the more complicated piece of this is adding tests for it. This could add a good bit of complexity to the current test suite as it could require simulating click/touch events to verify that the callbacks work, which the test suite does not currently do as no code has currently been added on top of signature_pad with regard to those events. The suggested change would in fact add code on top of signature_pad, thereby now requiring a test for that.

In any case, I hope that makes it clear that it is not quite simple to upgrade and there are many considerations to be made and testing to be done. As a fellow maintainer, @UziTech I think you might understand that most changes are often more complicated than they seem and have additional requirements crop up.

The other big breaking change is the structure of the data returned from toData. The main change their was a fix so each line segment has it's own properties for dotSize, minWidth, maxWidth (szimek/signature_pad#571) and the addition of pressure to the data points (szimek/signature_pad#566)

I'm not sure that this is actually breaking, but maybe I misunderstood something here. It's also listed as a fix in the changelog.
On this line, it only adds properties to the data, meaning it would be a backward-compatible, additive change. Unless fromData requires that structure now?

I do see a different breaking change however, which wasn't mentioned in the changelog or in the PR description. On this line, the type of dotSize was changed to only be a number and no longer allow a function. I understand that this is required so that dotSize can be stored in JSON (as a JS function would not be valid JSON), however it does seem to be breaking. That being said, I've actually never seen that functionality used before.

alternative -- monkey-patch v2?

An alternative I had hypothesized before, that may be more relevant given the number of changes to upgrade, would be to monkey-patch v2 in this library. It's hacky and would create a side-effect, but could be a good stop-gap fix.

Per szimek/signature_pad#601, the changes made are only at the top of the function, and the function is on the prototype, so I think it's possible to monkey-patch the prototype within this library to add those lines. There's a bit of complication with setting this properly, but it should be feasible.

I'd accept a monkey-patch change on top of the current dependency of v2. Might need a small test added for that, but if it could be manually verified that might also be acceptable for such a small hotfix that is intended to be a stop-gap until the upgrade.

I think that would be significantly preferable to forking, IMO. That being said, I'd have to agree with @M1ke 's comments with regard to "no changes downstream" as there are other wrapper libraries and forks (e.g. for Vue, etc) that would have to make similar changes for this fix.

[UziTech]

imho monkey patching is no different than forking. It means you will need to continue to maintain and fix bugs without downstream support.

I understand the downsides of bumping the major version but sometimes that is the best way to move forward.

[M1ke]

I agree with @UziTech that the monkey patch and fork mechanisms are not ideal. However the PR onto v2 of the signature_pad library was declined, so the least-work fix for this issue is no longer possible.

It seems that updating this library to use signature_pad v4, just for this one issue, would be excessive work. So the choice is between monkey patching and forking.

On that subject, my preference is for a fork. A fork gives more overall clarity as to what's happening, and makes any maintenance commitments of said fork clearer to consumers of the library. I'd suggest that given the age of signature_pad v2, it's unlikely that a lot of issues will come about that people are unaware of, and this one fix seems the clearest one to backport to v2.

I'd be willing to fork signature_pad v2, apply that fix and roll a version to npm, if @agilgur5 is in agreement that a PR for react-signature-canvas which moved the dependency from signature_pad to @m1ke/signature_pad-v2 would be accepted. I think that offers more clarity and less risk of mistakes than a monkey patch at this stage; but happy for an in-library solution that doesn't involve an external fork if you feel that's better.

(once we have a resolution we should also close this PR as we may as well open a new one to handle moving the dependency to a fork)

[UziTech]

It seems that updating this library to use signature_pad v4, just for this one issue, would be excessive work.

It wouldn't be for this one issue. There have been many fixes since v2. And it would be to make sure any future fixes get in as well.

[agilgur5]

monkey-patching vs forking

imho monkey patching is no different than forking. It means you will need to continue to maintain and fix bugs without downstream support.

That's implied and understood of course. Monkey-patching is just perhaps a better temporary resolution than forking.

We have been discussing temporary solutions this whole time as upgrading to v4 is not something that is going to happen overnight given its significant complexity (multiple breaking changes), and some users may be satisfied by a temporary solution in the meantime.

moved the dependency from signature_pad to @m1ke/signature_pad-v2 would be accepted. I think that offers more clarity and less risk of mistakes than a monkey patch at this stage; but happy for an in-library solution that doesn't involve an external fork if you feel that's better.

@M1ke it may offer "more clarity" in one sense of the word, but in another, such as in the sense of "trust" in an ecosystem with increasing amounts of supply chain attacks (and poor permissions against them), it can offer less of that. I wouldn't want users to be suspicious of react-signature-canvas because it started using a new, forked dep all of a sudden -- in particular, that's a very common attack pattern.

I do agree about less risk of mistakes too though, namely as an issue with two canvases popped up, and monkey-patching in that scenario would have to be a bit more complicated.

We could also use patch-package, which has a slightly different set of trade-offs (a postinstall hook in particular)

bumping major versions and the impact on the ecosystem

I understand the downsides of bumping the major version but sometimes that is the best way to move forward.

I don't disagree with this sentiment generically, but also as with any library, one can expect that downstream libraries will take time to upgrade. This becomes significantly more complicated when compatibility issues arise. And if those compatibility issues are complex, as is the case here (multiple breaking changes), one can further expect that the upgrade process will take exponentially longer as a result.

I think it would be worthwhile to add some concrete data here. Based on NPM download count, react-signature-canvas makes up ~1/3 of signature_pad's usage, and if you include various forks etc, it might be closer to 1/2.
Now that NPM shows download counts per version, you can also see in the Versions tab that 2.3.2 is overwhelmingly the most used version (~65%) and the entire 4.x.x line actually only has a small fraction of usage (~6%), despite having been out for ~5 months. The 1.x.x line has around ~3% usage still, for comparison.
So the data, even when taken with a grain of salt as raw download counts, overwhelmingly supports my "maintainer assumptions" statement above.

Also for reference, this was one the first open-source repos I maintained and I back-ported multiple patches between v0.3.x, v0.2.x, and v0.1.x, because those versions are still used.

In light of all those facts, I would think one could say that the one sentence rejection of a simple backport PR seems overly simplistic.

other breaking changes not responded to

@UziTech could you respond to my questions around the other breaking changes from my previous comment? I think that would be to the benefit of our shared userbase, and I can make a PR to the changelog to make any necessary corrections.

Specifically these two blocks:

I'm not sure that this is actually breaking, but maybe I misunderstood something here. It's also listed as a fix in the changelog. On this line, it only adds properties to the data, meaning it would be a backward-compatible, additive change. Unless fromData requires that structure now?

I do see a different breaking change however, which wasn't mentioned in the changelog or in the PR description. On this line, the type of dotSize was changed to only be a number and no longer allow a function.

EDIT: Added one PR szimek/signature_pad#616 (comment) so far to fix the changelog

[UziTech]

I'm not sure I'm the one you need to convince. I am just helping maintain the latest version of signature_pad. If you want to maintain a fork of v2 that is on you and if you want to maintain it in the repo https://github.com/szimek/signature_pad you will have to convince @szimek.

[UziTech]

For what it's worth I'm still convinced the best option is to update to signature_pad v4 and release a major update. Most of your users will stay on the old version but that is not your problem (for most of them it is no problem at all). They will update when the time is right for them. And for the vast majority of them there shouldn't actually be any breaking changes (assuming a successful wrapper is created for event functions).

Also NPM is not the only way signature_pad is used

[agilgur5]

stats

Also NPM is not the only way signature_pad is used

Yes, I'm aware of that. I've even helped track down some CDNJS issues in signature_pad (szimek/signature_pad#609)

Also the download counts from jsDeliver actually even further bolster my point; 2.3.2 is still ~90% of monthly downloads, while 4.0.0 is only ~5% of monthly downloads (and 4.0.5 is only ~0.5%).

old versions

Most of your users will stay on the old version but that is not your problem

I don't necessarily agree with this statement. If most of your users are on an old version, then your library has an upgrade adoption issue. That is still an issue within the purview of maintainers. (one of the reasons why maintainers have too much responsibility).
That's also one of the reasons many maintainers strive to make things as backward-compatible as possible. Some even make automated codemods for upgrades -- which is quite literally maintainers making upgrading their problem. There are some very popular libraries that do this, so this blanket statement just doesn't match reality.

(assuming a successful wrapper is created for event functions).

Given that no one's stepped up to do this so far, its high complexity as already discussed, and that I've been the only major contributor for years, this is expecting quite a lot.
I have no timeline for this, but have gotten multiple requests to upgrade to v4. It is unfortunately not at all simple due to the multiple breaking changes and this requirement. Not to mention that some of those breaking changes were not properly documented either, as illustrated above. So that means this library won't/hasn't upgraded for some time.

If you want to maintain a fork of v2

To be clear, back-porting is not the same as forking. Back-ports are very common in open-source, from kernels like the Linux Kernel to OSes to browsers to Node.js itself. For the larger components, these often have the concept of LTS (long-term support). Many JS libraries may not have LTS, but still back-port fixes, including, as mentioned above, react-signature-canvas itself.
So, again, back-porting fixes is not the same as forking, and is in fact a general norm that many libraries follow. Back-porting is done literally so that users who cannot yet make breaking upgrades are still able to get bugfixes and security patches (sometimes even new features as well). Back-porting is also often taken up by the same maintainers (and often it can be just a git cherry-pick away), whereas forking often means different maintainers.

other breaking changes not responded to

I still haven't gotten answers to these questions despite multiple asks 😕 😐

EDIT: Added one PR szimek/signature_pad#616 (comment) so far to fix the changelog

^This was merged, so I guess this has been self-answered now.

I'm not sure that this is actually breaking, but maybe I misunderstood something here. It's also listed as a fix in the changelog. On this line, it only adds properties to the data, meaning it would be a backward-compatible, additive change. Unless fromData requires that structure now?

^I'm still not sure the answer to this

back-porting fixes to v2

I'm not sure I'm the one you need to convince. I am just helping maintain the latest version of signature_pad. If you want to maintain [...] v2 that is on you and if you want to maintain it in the repo https://github.com/szimek/signature_pad you will have to convince @szimek.

That's fine to have that policy, but this is moving the goalposts. You were arguing for a certain side and did yourself reject @M1ke 's PR to the v2 branch in szimek/signature_pad#601. You didn't leave that up to @szimek initially, but are only now saying that.

For what it's worth, I very much agree with @M1ke 's point in szimek/signature_pad#601 (comment) that this is a very simple back-port that will fix a bug for 90% of users, because 90% of users have yet to upgrade to v4.

Back-porting generally is not that difficult (unlike this upgrade with multiple breaking changes, some of which were not properly documented) and @M1ke literally already made the PR for this work.
I can certainly help with back-porting as well and can probably back-port most of the fixes in a day.
The blocker here is not the back-ports themselves -- which I think is a pretty critical point -- the blocker is that those back-ports have been / will be rejected.

If @szimek wants to add me as a maintainer to back-port fixes to v2 for the time-being, I can certainly do that. And can help merge PRs to v2 from @M1ke or anyone else. In terms of "trust", I already maintain many open-source libraries and am already a long-time contributor to the ecosystem (react-signature-canvas has been around for like ~6 years, trim-canvas is also written by me, and nowadays I help maintain rollup-plugin-typescript2 that signature_pad uses as well).
However, I'm aware that @szimek is not as active here anymore.

I am not able to add myself, so that is currently outside of my control (and publishing releases to NPM is also a separate permission). The only thing we can do is create a fork, which as mentioned above, will cause trust/security issues in the community, so that is not a particularly good option either.

[M1ke]

Comment level: heroic!

Seriously, let's get this done. Get the PR into V2. If not, I'll fork signature pad V2 so @agilgur5 doesn't have to seem like the one breaking cohesiveness in an ecosystem they're much deeper into than I am; then I'll pr this repo to change the dependency. Hopefully we don't need to.

But this bug will be annoying such a range of users, especially those using error logging tools getting all that noise - we'll be doing a service by fixing it, even if we can't fix it in the "ideal" way

[UziTech]

There are some very popular libraries that do this

The only ones I know of are beholden to sponsors. Last I checked signature_pad (as well as this library) doesn't have any of those.

I'm not telling you can't or shouldn't support your users that stay on older versions, I'm saying you don't have to and there will be very few complaints if you don't.

To be clear, back-porting is not the same as forking.

It requires the same amount of effort to maintain. And again it isn't me you need to convince if you would like to maintain a "backport" for v2 on szimek/signature_pad.

I still haven't gotten answers to these questions despite multiple asks

That is because no one knows the answers. If you would like to look through the repo to figure out the answers no one is stopping you.

Honestly I haven't used signature_pad on a daily basis for almost a year now so I have to reread the code to remind myself how it works every time there is an issue or pr. @szimek walked away from the project a long time ago. I think the best bet is for one or both of you two to fork it and maintain a separate copy. It is pretty much in maintenance mode now just getting bug fixes whenever anyone finds a bug. It has been feature complete for a while.

I am not able to add myself, so that is currently outside of my control

Mine as well. I don't have access to add people or upload to npm.

[agilgur5]

regarding maintenance

The only ones I know of are beholden to sponsors.

Off the very top of my head I can think of at least two:

• node-notifier, which has 40M monthly downloads, has done multiple backports
• rollup-plugin-terser, ~10M monthly, has done at least one backport too

and, as you observe, neither is react-signature-canvas at ~400k monthly, which has many backports (even the current release, v1.0.6, is actually a backport of the yet unreleased v1.1.0 TS and internal tooling re-write).

There are certainly more, that's just ones that immediately come to mind as backports that I have literally used myself in the past (in this case, I remembered these raising issues while I was maintaining TSDX).

I'm not telling you can't or shouldn't support your users that stay on older versions, I'm saying you don't have to and there will be very few complaints if you don't.

maybe. this is personal opinion. whether that is best practice I think is a separate story.

But yea, absolutely, OSS relies on volunteers so it should be expected that not every best practice is followed, since, hell, paid developers at corporations don't even do that. Both signature_pad and react-signature-canvas only added things like tests, TypeScript, CI, etc after years of existence. And I still haven't added a Storybook here.

And, to be clear, it's also not that I expect you to backport things to v2 yourself -- we've got two contributors right here after all -- it's just that I think that would have been significantly less effort to merge a few PRs and make one release than this entire thread, IMO.

It requires the same amount of effort to maintain.

Just from personal experience, I disagree. As I said above, cherry-picking doesn't actually tend to be that difficult (except for big ranges, ofc). It's still a pain to make releases, but if those are automated or batched into a single release, as we could do here, that decreases the load quite heavily.

It is pretty much in maintenance mode now just getting bug fixes whenever anyone finds a bug. It has been feature complete for a while.

Especially in the case where there are only bugfixes, this generally makes it pretty easy to cherry-pick as it is roughly 1-to-1 (the TS portions complicate it a bit though, but these don't change that many lines at all).

regarding other breaking changes

That is because no one knows the answers. If you would like to look through the repo to figure out the answers no one is stopping you.

Er, well... you brought this up yourself.... so I would say that me asking for clarification (x3) about your own statement which contradicts your own changelog is a pretty reasonable question, no??

That changelog entry and those PRs were written by you as well, so I also think it makes sense to ask the author when they themselves bring it up, no??

I did look through for my first changelog PR, this one was a lot more confusing though, and the contradiction of the changelog just served to confuse me more. Like legit confused here, and that is what I am trying to convey in my tone/wording (not sure if that comes out well over text).

Honestly I haven't used signature_pad on a daily basis for almost a year now so I have to reread the code to remind myself how it works every time there is an issue or pr.

Understood, I have to do that with various libraries of mine too.

I do see that you respond to issues/discussions pretty often (and much respect to you for that too!! 👍 💯 one of the least appreciated pieces of being a maintainer ❤️ ) since I occasionally hop in myself.
So given that and that you had brought it up yourself here and wrote it, I genuinely thought you would know the answer off the top of your head. Apologies if I was bugging you when you literally didn't know, I didn't anticipate that was the case when you hadn't responded to the requests for clarification; I thought you just missed them or something.

next steps

@szimek walked away from the project a long time ago.

Yeaaa that's kind of why I was taken aback when you kicked the can to @szimek, if we're being real here 😅

Mine as well. I don't have access to add people or upload to npm.

Ah, ok. I thought @szimek would've granted you those permissions since you're pretty much the only maintainer and he hasn't been around.

That would've been great to know initially too as a short-circuit, since the request to backport effectively becomes impossible without @szimek .

I think the best bet is for one or both of you two to fork it and maintain a separate copy

Sounds like a plan then, appreciate your responses here @UziTech . (and how incredibly quick you respond!)

If not, I'll fork signature pad V2 so @agilgur5 doesn't have to seem like the one breaking cohesiveness in an ecosystem they're much deeper into than I am

Since we do have to go with this method, let's use my fork, just so that users have more trust if they see @agilgur5/signature_pad vs. @M1ke/signature_pad.
I technically already have a "fork" on GitHub, will just need to update the v2 branch with some docs indicating the backports and different name. I'll of course point people to upgrading to official v4 instead as the preferred option as well.

But this bug will be annoying such a range of users, especially those using error logging tools getting all that noise - we'll be doing a service by fixing it, even if we can't fix it in the "ideal" way

yea seeing #90 brought me back here

Comment level: heroic!

and thank you, I try to explain things in meticulous detail for all readers and posterity!
appreciate the people that acknowledge that effort! ❤️

(which is a very welcome contrast to the toxicity you find in much of OSS. that makes OSS more welcoming and positive instead of depressing. I cannot overstate this enough given that I literally keep a private OSS gratitude journal to help keep me going)

[M1ke]

Sounds like a plan. I'll send a new pr for the backported fix into your fork instead. I may as well let you repoint the dependency though.