fix: Replace usage of useEvent with useCallback

[pawelgrimm]

Overview

A function created with the useEvent hook was being used in TypistEditor's onCreate function, which was called during rendering (when initializing the editor useState in useEditor). This caused it to throw an error when React's Strict Mode was enabled, as this type of usage makes the rendering result non-determinisitic. See reactjs/rfcs#220 (comment) for further discussion.

PR Checklist

• Pull request title follows the Conventional Commits Specification

[netlify]

✅ Deploy Preview for doist-typist ready!

Name	Link
🔨 Latest commit	0cb3d52
🔍 Latest deploy log	https://app.netlify.com/sites/doist-typist/deploys/63f670999283f20008b7deec
😎 Deploy Preview	https://deploy-preview-98--doist-typist.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[pawelgrimm]

@rfgamaral What do you think about this change? Can you share any context around why we opted for useEvent here in the first place?

[rfgamaral]

Can you share any context around why we opted for useEvent here in the first place?

Before using useEvent we were using useRefCallback (ref), which was our own custom version of useEvent. We moved to useEvent when Typist became open-source, and to lose the dependency on the private @doist/toolbox-react-hooks package.

With that said, I was first introduced to useRefCallback by Ernesto in this thread, and although that was mainly a question for event handlers passed on to <TypistEditor> (at the time it was called RichTextEditor), I somehow felt the need to use it in the onCreate handler inside <TypistEditor>.

I don't have more context than that, and useCallback might be sufficient here, however, I have some questions before we consider merging this, but I believe https://github.com/Doist/todoist-web/pull/5048 to be the more appropriate place to discuss that.

[pawelgrimm]

@rfgamaral My biggest concern here is that we don't introduce some weird behavior like the editor state being erased during normal usage because of this change (which, IIRC, is why we started using useRefCallback in the first place).

[rfgamaral]

@pawelgrimm I honestly cannot say if that will be the case or not. I don't think we'll introduce weird behaviour with this change, because like I said, I added it here "just because", no good reason, and probably shouldn't have.

However, to avoid going back and forth with Typist releases (and updating Todoist), may I suggest that we apply a patch on todoist-web with this change and validate that way? We can have one (or more) people from the team validate the PR, and then we'll also have our users validating the change for us. If we don't get any tickets in a day, we are probably safe 😅

If you go that route, please be careful with the current patch, and keep it around, it's still needed.

[rfgamaral]

Since useEvent is no longer used on Typist itself, we also need to move "react-use-event-hook": "^0.9.3", from peerDependencies to devDependencies (don't forget to remove the ^ from the version number when moving it to devDependencies, and make sure the alphabetical order is kept).

[rfgamaral]

@pawelgrimm I think we can proceed with this, don't you think?

As a reminder, don't forget to do the change I requested above, and also change the PR title from refactor: to fix:. Technically, it's a fix, right? Either way, refactor: won't trigger a new release automatically, and fix: will, which is what we want after we merge this.

Then feel free to update Todoist with the new version (and don't forget to revert the patch to the previous version).

Change was implemented as requested

[doistbot]

🎉 This PR is included in version 1.0.16 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀