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.

Sign up for GitHub

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

Jump to bottom

Interactivity API: Refactor context proxies #64713

Merged
merged 11 commits into from
Sep 17, 2024
Merged

Interactivity API: Refactor context proxies #64713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3cfcda7
Move `proxifyContext` to proxies folder
DAreRodz Aug 21, 2024
9853ca3
Move context handlers outside
DAreRodz Aug 21, 2024
c40acbd
Remove unused code
DAreRodz Aug 22, 2024
0f12399
Update comment
DAreRodz Aug 22, 2024
8859cd5
Add TODO comment
DAreRodz Aug 22, 2024
dc16445
Remove unused code and rename `k` arg to `key`
DAreRodz Aug 26, 2024
ad6a974
Replace `updateContext` with `deepMerge`
DAreRodz Sep 8, 2024
d09316d
Throw when calling `proxifyContext` on a context proxy
DAreRodz Sep 8, 2024
2c17bf3
Add unit tests
DAreRodz Sep 8, 2024
2392742
Update changelog
DAreRodz Sep 8, 2024
d11f743
Added new tests
michalczaplinski Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/interactivity/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Enhancements

- Refactor internal context proxies implementation ([#64713](https://github.com/WordPress/gutenberg/pull/64713)).

### Bug Fixes

- Prevent calling `proxifyContext` over an already-proxified context inside `wp-context` ([#65090](https://github.com/WordPress/gutenberg/pull/65090)).
Expand Down
135 changes: 4 additions & 131 deletions packages/interactivity/src/directives.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@
*/
import { h as createElement, type RefObject } from 'preact';
import { useContext, useMemo, useRef } from 'preact/hooks';
/**
* Internal dependencies
*/
import { proxifyState, peek } from './proxies';

/**
* Internal dependencies
Expand All @@ -24,131 +20,7 @@ import {
} from './utils';
import { directive, getEvaluate, type DirectiveEntry } from './hooks';
import { getScope } from './scopes';

// Assigned objects should be ignored during proxification.
const contextAssignedObjects = new WeakMap();

// Store the context proxy and fallback for each object in the context.
const contextObjectToProxy = new WeakMap();
const contextProxyToObject = new WeakMap();
const contextObjectToFallback = new WeakMap();

const descriptor = Reflect.getOwnPropertyDescriptor;

/**
* Wrap a context object with a proxy to reproduce the context stack. The proxy
* uses the passed `inherited` context as a fallback to look up for properties
* that don't exist in the given context. Also, updated properties are modified
* where they are defined, or added to the main context when they don't exist.
*
* By default, all plain objects inside the context are wrapped, unless it is
* listed in the `ignore` option.
*
* @param current Current context.
* @param inherited Inherited context, used as fallback.
*
* @return The wrapped context object.
*/
const proxifyContext = ( current: object, inherited: object = {} ): object => {
// Update the fallback object reference when it changes.
contextObjectToFallback.set( current, inherited );
if ( ! contextObjectToProxy.has( current ) ) {
const proxy = new Proxy( current, {
get: ( target: object, k: string ) => {
const fallback = contextObjectToFallback.get( current );
// Always subscribe to prop changes in the current context.
const currentProp = target[ k ];

// Return the inherited prop when missing in target.
if ( ! ( k in target ) && k in fallback ) {
return fallback[ k ];
}

// Proxify plain objects that were not directly assigned.
if (
k in target &&
! contextAssignedObjects.get( target )?.has( k ) &&
isPlainObject( currentProp )
) {
return proxifyContext( currentProp );
}

// Return the stored proxy for `currentProp` when it exists.
if ( contextObjectToProxy.has( currentProp ) ) {
return contextObjectToProxy.get( currentProp );
}
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved

/*
* For other cases, return the value from target, also
* subscribing to changes in the parent context when the current
* prop is not defined.
*/
return k in target ? currentProp : fallback[ k ];
},
set: ( target, k, value ) => {
const fallback = contextObjectToFallback.get( current );
const obj =
k in target || ! ( k in fallback ) ? target : fallback;

/*
* Assigned object values should not be proxified so they point
* to the original object and don't inherit unexpected
* properties.
*/
if ( value && typeof value === 'object' ) {
if ( ! contextAssignedObjects.has( obj ) ) {
contextAssignedObjects.set( obj, new Set() );
}
contextAssignedObjects.get( obj ).add( k );
}

/*
* When the value is a proxy, it's because it comes from the
* context, so the inner value is assigned instead.
*/
if ( contextProxyToObject.has( value ) ) {
const innerValue = contextProxyToObject.get( value );
obj[ k ] = innerValue;
} else {
obj[ k ] = value;
}
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved

return true;
},
ownKeys: ( target ) => [
...new Set( [
...Object.keys( contextObjectToFallback.get( current ) ),
...Object.keys( target ),
] ),
],
getOwnPropertyDescriptor: ( target, k ) =>
descriptor( target, k ) ||
descriptor( contextObjectToFallback.get( current ), k ),
} );
contextObjectToProxy.set( current, proxy );
contextProxyToObject.set( proxy, current );
}
return contextObjectToProxy.get( current );
};

/**
* Recursively update values within a context object.
*
* @param target A context instance.
* @param source Object with properties to update in `target`.
*/
const updateContext = ( target: any, source: any ) => {
for ( const k in source ) {
if (
isPlainObject( peek( target, k ) ) &&
isPlainObject( source[ k ] )
) {
updateContext( peek( target, k ) as object, source[ k ] );
} else if ( ! ( k in target ) ) {
target[ k ] = source[ k ];
}
}
};
import { proxifyState, proxifyContext, deepMerge } from './proxies';

/**
* Recursively clone the passed object.
Expand Down Expand Up @@ -286,9 +158,10 @@ export default () => {
`The value of data-wp-context in "${ namespace }" store must be a valid stringified JSON object.`
);
}
updateContext(
deepMerge(
currentValue.current,
deepClone( value ) as object
deepClone( value ) as object,
false
Comment on lines -289 to +164
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:

    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 ];
}

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.

);
result[ namespace ] = proxifyContext(
currentValue.current,
Expand Down
69 changes: 69 additions & 0 deletions packages/interactivity/src/proxies/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
const contextObjectToProxy = new WeakMap();
const contextObjectToFallback = new WeakMap();
const contextProxies = new WeakSet();

const descriptor = Reflect.getOwnPropertyDescriptor;

// TODO: Use the proxy registry to avoid multiple proxies on the same object.
const contextHandlers: ProxyHandler< object > = {
get: ( target, key ) => {
const fallback = contextObjectToFallback.get( target );
// Always subscribe to prop changes in the current context.
const currentProp = target[ key ];

/*
* Return the value from `target` if it exists, or from `fallback`
* otherwise. This way, in the case the property doesn't exist either in
* `target` or `fallback`, it also subscribes to changes in the parent
* context.
*/
return key in target ? currentProp : fallback[ key ];
},
set: ( target, key, value ) => {
const fallback = contextObjectToFallback.get( target );

// If the property exists in the current context, modify it. Otherwise,
// add it to the current context.
const obj = key in target || ! ( key in fallback ) ? target : fallback;
obj[ key ] = value;

return true;
},
ownKeys: ( target ) => [
...new Set( [
...Object.keys( contextObjectToFallback.get( target ) ),
...Object.keys( target ),
] ),
],
getOwnPropertyDescriptor: ( target, key ) =>
descriptor( target, key ) ||
descriptor( contextObjectToFallback.get( target ), key ),
};

/**
* Wrap a context object with a proxy to reproduce the context stack. The proxy
* uses the passed `inherited` context as a fallback to look up for properties
* that don't exist in the given context. Also, updated properties are modified
* where they are defined, or added to the main context when they don't exist.
*
* @param current Current context.
* @param inherited Inherited context, used as fallback.
*
* @return The wrapped context object.
*/
export const proxifyContext = (
current: object,
inherited: object = {}
): object => {
if ( contextProxies.has( current ) ) {
throw Error( 'This object cannot be proxified.' );
}
// Update the fallback object reference when it changes.
contextObjectToFallback.set( current, inherited );
if ( ! contextObjectToProxy.has( current ) ) {
const proxy = new Proxy( current, contextHandlers );
contextObjectToProxy.set( current, proxy );
contextProxies.add( proxy );
}
return contextObjectToProxy.get( current );
};
1 change: 1 addition & 0 deletions packages/interactivity/src/proxies/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
*/
export { proxifyState, peek, deepMerge } from './state';
export { proxifyStore } from './store';
export { proxifyContext } from './context';
Loading
Loading