Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useResizeObserver: handle strict mode double effects #47703

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Feb 2, 2023

Fixes useResizeObserver in strict mode. Which unmounts and remounts effects twice, and the second mount didn't update didUnmount.current back to false, so the hook was considered permanently unmounted.

@jsnajdr jsnajdr requested a review from ntsekouras February 2, 2023 17:20
@jsnajdr jsnajdr requested a review from ajitbohra as a code owner February 2, 2023 17:20
@jsnajdr jsnajdr self-assigned this Feb 2, 2023
@jsnajdr jsnajdr requested a review from tyxla February 2, 2023 17:22
@ntsekouras
Copy link
Contributor

ntsekouras commented Feb 2, 2023

I tested only in local dev mode for now.

  1. The first issues I observed with site editor and previews seem fine
  2. The second issue with history(undo/redo) in post editor persists.
Screen.Recording.2023-02-02.at.7.42.53.PM.mov

There might be more issues though.. I haven't tested thoroughly..

@ntsekouras
Copy link
Contributor

Also saw this one:
Screenshot 2023-02-02 at 7 51 59 PM

@ciampo
Copy link
Contributor

ciampo commented Feb 2, 2023

I confirm what @ntsekouras says — the site editor is back, but sometimes the undo button does not activate, and the "restore" banner appears twice.

As Nik mentions, there could likely be more issues that we may find by testing this PR.

Maybe the better idea is to revert the offending PR, and work on a fix with less urgency? I'm just afraid that we'd introduce more regressions without careful testing

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably merge this hotfix while still reverting the original Strict Mode PR

@jsnajdr jsnajdr merged commit ba64cd4 into trunk Feb 2, 2023
@jsnajdr jsnajdr deleted the fix/use-resize-observer-strict-mode branch February 2, 2023 18:26
@github-actions github-actions bot added this to the Gutenberg 15.2 milestone Feb 2, 2023
@WunderBart
Copy link
Member

Do we need to add regression tests for this? I guess it should have been caught in the original PR (where we enabled the Strict Mode).

@jsnajdr
Copy link
Member Author

jsnajdr commented Feb 3, 2023

Do we need to add regression tests for this?

To catch this, we'd have to run the e2e tests against a development build. In production mode, strict mode doesn't do anything. Only in dev it does things like re-mounting effects, exposing bugs like this one.

@jsnajdr
Copy link
Member Author

jsnajdr commented Feb 3, 2023

but sometimes the undo button does not activate

There is some bug in the useBlockSync hook. When a block changes in core/block-editor, this hook should sync the change to a core entity, calling editEntityRecord. And it's the core store that maintains the undo/redo stack. Sometimes, this sync doesn't happen. Going to investigate this. useBlockSync is quite complex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Compose /packages/compose [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants