search

Automattic / simplenote-electron

Simplenote for Web, Windows, and Linux
https://app.simplenote.com
GNU General Public License v2.0
4.69k stars 553 forks source link

Update: react-monaco-editor and the hooks issue #3201

Open codebykat opened 1 month ago

codebykat commented 1 month ago

Fix

In the course of the multiple upgrades in #3183, we noticed that the editor sometimes failed to focus the text area on load or on creating a new note. I eventually traced this to the version bump of monaco-react-editor, specifically the refactor to use hooks, which was included in versions higher than 0.50.1.

After some experimentation I reordered the code in such a way that it seems to work (moving the initialization from componentDidMount into editorReady), but I'm a little worried about this change, because the existing codebase is written with a Redux (action dispatch) architecture that largely predates React hooks, and I'm not sure whether it might introduce unintended side effects. I'm also not sure if we still need to use a boot timer, or if this refactor incidentally solves that.

The discussion at the above-referenced issue does not contain any real workarounds nor is there an update to the component that reverts or fixes the behavior, which is somewhat concerning. However a quick search for alternatives to react-monaco-editor didn't unearth any viable options (the other major package is written to load the editor from a CDN on load to avoid configuration, which is definitely a choice).

Test

Since this change affects the loading order of the editor component, we need to make sure it doesn't have any unintended side effects, so smoke-test all the things: switching notes, creating a new note, syncing changes, etc.

Release

Fixed a bug causing the text area not to be focused on load or on new note creation.

codebykat commented 1 month ago

Thanks @dmsnell!

I don't remember the reason it's there but I would hope it's in a commit message.

Unfortunately, as with so many things around here, almost all the blame points to a giant changeset that was itself a refactor of something else: https://github.com/Automattic/simplenote-electron/commit/fc8921291b6b702db35b076d65e4632203ff4574 🫠

dmsnell commented 1 month ago

@codebykat the answer is in the following

Screenshot 2024-05-23 at 11 56 42 AM

https://github.com/Automattic/simplenote-electron/blame/d036a087370589a884f03ef32955e1842caf26ac/lib/note-content-editor.tsx#L126-L137

We were dealing with UI jank when navigating through notes because of the shift that occurs when initializing Monaco. It would render the checkboxes and that bogged down scrolling. The initial state then was the "fast" mode, which rendered plaintext, and when stopping at a single note, after the delay, would shift into "full" mode with the replaced checkboxes and full Monaco functionality.

So it's not necessary if the experience of switching between notes remains fast. It's a workaround to not having a faster UI.