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

Interactivity API: Fix reactivity of undefined objects and arrays added with deepMerge #66183

Merged
merged 6 commits into from
Oct 18, 2024

Conversation

DAreRodz
Copy link
Contributor

@DAreRodz DAreRodz commented Oct 16, 2024

What?

Fixes a regression that breaks the reactivity of props from lazily instantiated objects/arrays inside the iAPI state or context.

The bug affects all directives reading properties from any lazily instantiated objects, e.g., those created by a module loaded asynchronously or during client-side navigation, impacting developers with such cases.

At this moment, it affects the loading bar that the @wordpress/interactivity-router module shows during navigations, which currently requires the following PHP code to work correctly (link), although it shouldn't be necessary:

/*
 * Initialize the `core/router` store.
 * If the store is not initialized like this with minimal
 * navigation object, the interactivity-router script module
 * errors.
 */
$this->state(
	'core/router',
	array(
		'navigation' => new stdClass(),
	)
);

Props to @sirreal for pointing this out in sirreal/wordpress-develop#8.

Why?

The bug could affect developers whose blocks depend on lazily instantiated objects/arrays inside the iAPI state or context.

How?

This PR fixes the deepMerge() function, ensuring it "proxifies" newly created objects and arrays so they become "reactive".

Testing Instructions

The following instructions should be followed in a WP instance where this lines are removed.

To reproduce the issue:

  1. Check out trunk
  2. In the site editor, ensure the homepage contains a Query block with the "Force page reload" setting disabled.
  3. Ensure you have enough posts to have more than one page in the Query block.
  4. Visit the homepage of your dev site.
  5. Log out to hide the admin top bar.
  6. Open the browser dev tools. In the Network tab, set a slow throttling preset so the next page takes some time to load.
  7. In the pagination block, click on "next page".
  8. Check that the top bar doesn't appear.

To ensure the issue is fixed:

  1. Check out this PR
  2. Repeat previous steps 2 to 7.
  3. Check that the top bar now appears as expected.

@DAreRodz DAreRodz added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) [Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity labels Oct 16, 2024
@DAreRodz DAreRodz self-assigned this Oct 16, 2024
@DAreRodz DAreRodz marked this pull request as ready for review October 16, 2024 19:15
@DAreRodz DAreRodz requested a review from luisherranz as a code owner October 16, 2024 19:15
Copy link

github-actions bot commented Oct 16, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: michalczaplinski <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

Flaky tests detected in ca60179.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11372332281
📝 Reported issues:

@DAreRodz DAreRodz force-pushed the fix/iapi-deep-merge-undefined-objects-reactivity branch from ca60179 to bf38930 Compare October 17, 2024 09:17
Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

This looks good to me. I would only update one of the tests as noted above.

On a somewhat related note: I would add some comments to the deepMergeRecursive() implementation because it's a complex beast 😄 . I'll send a PR with this.

@michalczaplinski
Copy link
Contributor

On a somewhat related note: I would add some comments to the deepMergeRecursive() implementation because it's a complex beast 😄 . I'll send a PR with this.

Here it is: #66220

I also had some doubts about whether deepMerge() was compatible with arrays so I added a few additional tests in: #66218

@DAreRodz
Copy link
Contributor Author

Great additions, @michalczaplinski. I'll merge them. ⭐

@DAreRodz DAreRodz enabled auto-merge (squash) October 18, 2024 10:27
@DAreRodz DAreRodz merged commit 6c0bca1 into trunk Oct 18, 2024
63 of 64 checks passed
@DAreRodz DAreRodz deleted the fix/iapi-deep-merge-undefined-objects-reactivity branch October 18, 2024 10:54
@github-actions github-actions bot added this to the Gutenberg 19.6 milestone Oct 18, 2024
Copy link

There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch.

PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk.

# Checkout the wp/6.7 branch instead of trunk.
git checkout wp/6.7
# Create a new branch for your PR.
git checkout -b my-branch
# Cherry-pick the commit.
git cherry-pick 6c0bca194715f0624be38a0df2cde3d5c970a881
# Check which files have conflicts.
git status
# Resolve the conflict...
# Add the resolved files to the staging area.
git status
git add .
git cherry-pick --continue
# Push the branch to the repository
git push origin my-branch
# Create a PR and set the base to the wp/6.7 branch.
# See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request.

@getdave
Copy link
Contributor

getdave commented Oct 18, 2024

@DAreRodz It looks like this didn't cherry pick cleanly to the wp/6.7 branch. It would be super helpful if you could take point on manually backporting this.

I will be cutting the packages for RC 1 on Monday morning UTC so the backport PR would need to land before then.

Many thanks

DAreRodz added a commit that referenced this pull request Oct 18, 2024
…ed with `deepMerge` (#66183)

* Fix types in test

* Add failing tests

* Fix the bug

* Update changelog

* Fix changelog

* Update tests

Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: michalczaplinski <[email protected]>
@DAreRodz
Copy link
Contributor Author

@getdave, sure! This is the PR for the cherry-pick: #66236.

cbravobernal pushed a commit that referenced this pull request Oct 18, 2024
…ed with `deepMerge` (#66183) (#66236)

* Fix types in test

* Add failing tests

* Fix the bug

* Update changelog

* Fix changelog

* Update tests

Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: michalczaplinski <[email protected]>
@getdave getdave added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 18, 2024
@getdave
Copy link
Contributor

getdave commented Oct 18, 2024

Thanks so much @DAreRodz. I've updated the labels here accordingly now that #66236 merged.

priethor pushed a commit that referenced this pull request Oct 23, 2024
…ed with `deepMerge` (#66183)

* Fix types in test

* Add failing tests

* Fix the bug

* Update changelog

* Fix changelog

* Update tests

Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: michalczaplinski <[email protected]>
@priethor
Copy link
Contributor

I just cherry-picked this PR to the release/19.5 branch to get it included in the next release: 8492898

@priethor priethor removed the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Oct 23, 2024
@gziolo gziolo modified the milestones: Gutenberg 19.6, Gutenberg 19.5 Oct 23, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…ed with `deepMerge` (WordPress#66183)

* Fix types in test

* Add failing tests

* Fix the bug

* Update changelog

* Fix changelog

* Update tests

Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: michalczaplinski <[email protected]>
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Nov 18, 2024
…outer

Remove the workaround for a bug that was fixed in WordPress/gutenberg#66183. Previously, if the store was not initialized with a minimal navigation object, the interactivity-router script module would error.

Props jonsurrell, czapla, gziolo.
Fixes 62465#.


git-svn-id: https://develop.svn.wordpress.org/trunk@59416 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Nov 18, 2024
…outer

Remove the workaround for a bug that was fixed in WordPress/gutenberg#66183. Previously, if the store was not initialized with a minimal navigation object, the interactivity-router script module would error.

Props jonsurrell, czapla, gziolo.
Fixes 62465#.

Built from https://develop.svn.wordpress.org/trunk@59416


git-svn-id: http://core.svn.wordpress.org/trunk@58802 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to gilzow/wordpress-performance that referenced this pull request Nov 18, 2024
…outer

Remove the workaround for a bug that was fixed in WordPress/gutenberg#66183. Previously, if the store was not initialized with a minimal navigation object, the interactivity-router script module would error.

Props jonsurrell, czapla, gziolo.
Fixes 62465#.

Built from https://develop.svn.wordpress.org/trunk@59416


git-svn-id: https://core.svn.wordpress.org/trunk@58802 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@cbravobernal cbravobernal added Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release and removed Backported to WP Core Pull request that has been successfully merged into WP Core labels Nov 20, 2024
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Nov 20, 2024
…outer.

Remove the workaround for a bug that was fixed in WordPress/gutenberg#66183. Previously, if the store was not initialized with a minimal navigation object, the interactivity-router script module would error.

Reviewed by desrosj.
Merges [59416] to the 6.7 branch.

Props jonsurrell, czapla, gziolo.
Fixes #62465.

git-svn-id: https://develop.svn.wordpress.org/branches/6.7@59436 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Nov 20, 2024
…outer.

Remove the workaround for a bug that was fixed in WordPress/gutenberg#66183. Previously, if the store was not initialized with a minimal navigation object, the interactivity-router script module would error.

Reviewed by desrosj.
Merges [59416] to the 6.7 branch.

Props jonsurrell, czapla, gziolo.
Fixes #62465.
Built from https://develop.svn.wordpress.org/branches/6.7@59436


git-svn-id: http://core.svn.wordpress.org/branches/6.7@58822 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@cbravobernal cbravobernal removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity [Type] Bug An existing feature does not function as intended
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants