-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
There was a problem hiding this 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.
Thanks, David. Let me know once it's ready for review. |
b814b9b
to
7fb68a8
Compare
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
updateContext( | ||
deepMerge( | ||
currentValue.current, | ||
deepClone( value ) as object | ||
deepClone( value ) as object, | ||
false |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 🙂
There was a problem hiding this comment.
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:
-
We do not use
peek()
anymore. This I assume is because we the context is not wrapped in proxies anymore like you mentioned here. -
I see that we're now assigning empty object here:
gutenberg/packages/interactivity/src/proxies/state.ts
Lines 298 to 301 in ce56700
} 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 ]; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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. 🙂
- 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
.
ce56700
to
2392742
Compare
to validate behavior with proxified state. Also included more descriptive error messages for re-proxification attempts.
There was a problem hiding this 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.
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.