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: Refactor context proxies #64713

Merged
merged 11 commits into from
Sep 17, 2024
Merged

Conversation

DAreRodz
Copy link
Contributor

What?

Minor refactoring for the context proxies. This PR moves the related code to the /proxies folder inside the Interactivity API package and removes unnecessary code.

Follow up of #62734.

Why?

Code quality and maintainability.

How?

This PR keeps the previous logic; this means context objects still have two proxies. It's a bit tricky to merge the state proxies logic with the context proxies one, so we could address that in a future PR.

Testing Instructions

Current tests should pass as expected.

@DAreRodz DAreRodz added [Type] Enhancement A suggestion for improvement. [Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity labels Aug 22, 2024
@DAreRodz DAreRodz self-assigned this Aug 22, 2024
Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

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

It looks good so far. Thanks for refactoring this, @DAreRodz!

I have left some minor comments and questions here.

packages/interactivity/src/proxies/context.ts Outdated Show resolved Hide resolved
packages/interactivity/src/proxies/context.ts Outdated Show resolved Hide resolved
packages/interactivity/src/proxies/context.ts Outdated Show resolved Hide resolved
packages/interactivity/src/proxies/context.ts Outdated Show resolved Hide resolved
packages/interactivity/src/proxies/context.ts Outdated Show resolved Hide resolved
packages/interactivity/src/proxies/context.ts Outdated Show resolved Hide resolved
@luisherranz
Copy link
Member

Thanks, David. Let me know once it's ready for review.

@DAreRodz DAreRodz force-pushed the refactor/iapi-context-proxies branch 2 times, most recently from b814b9b to 7fb68a8 Compare September 7, 2024 12:23
@DAreRodz DAreRodz marked this pull request as ready for review September 8, 2024 18:30
Copy link

github-actions bot commented Sep 8, 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 <darerodz@git.wordpress.org>
Co-authored-by: michalczaplinski <czapla@git.wordpress.org>
Co-authored-by: luisherranz <luisherranz@git.wordpress.org>

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

@DAreRodz
Copy link
Contributor Author

DAreRodz commented Sep 8, 2024

The error that you see in the wp-each tests (here) should be gone once the bug fix in #65135 is merged. ☝️

packages/interactivity/src/directives.tsx Show resolved Hide resolved
Comment on lines -289 to +164
updateContext(
deepMerge(
currentValue.current,
deepClone( value ) as object
deepClone( value ) as object,
false
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me that deepMerge is equivalent to the updateContext() function. Should it be equivalent or are we consciously changing the logic here? If it's the second one, then why do we do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is to use deepMerge() with the override option set to false. This algorithm follows the same logic as updateContext but does not manipulate the reactive object directly and only updates the signals created beforehand.

That way, we avoid wp-context subscribing unnecessarily to the modified context and prevent wasteful signal instantiation. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I see. Upon further inspection the logic does seem almost the same. The only 2 differences I see is:

  1. We do not use peek() anymore. This I assume is because we the context is not wrapped in proxies anymore like you mentioned here.

  2. I see that we're now assigning empty object here:

    } else if ( isPlainObject( source[ key ] ) ) {
    if ( ! ( key in target ) ) {
    target[ key ] = {};
    }

    but previously we used to re-assign the property:

    } else if ( ! ( k in target ) ) {
      target[ k ] = source[ k ];
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. We do not use peek() anymore. This I assume is because we the context is not wrapped in proxies anymore like you mentioned here.

Exactly. 🙂

  1. I see that we're now assigning empty object here [...] but previously we used to re-assign the property:

That was wrong. We don't want to copy a plain object's reference from source when calling deepMerge.

packages/interactivity/src/directives.tsx Show resolved Hide resolved
@DAreRodz DAreRodz force-pushed the refactor/iapi-context-proxies branch from ce56700 to 2392742 Compare September 9, 2024 17:01
to validate behavior with proxified state. Also included more descriptive error messages for re-proxification attempts.
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.

Nice work @DAreRodz !

This looks good to me! 👍 I hope it's okay I pushed a tiny improvement to the tests.

@DAreRodz DAreRodz merged commit 636710b into trunk Sep 17, 2024
61 checks passed
@DAreRodz DAreRodz deleted the refactor/iapi-context-proxies branch September 17, 2024 17:42
@github-actions github-actions bot added this to the Gutenberg 19.3 milestone Sep 17, 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] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants