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 internal proxy and signals system #62734

Merged
merged 91 commits into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
91 commits
Select commit Hold shift + click to select a range
181841a
WIP
DAreRodz Jun 28, 2024
e78df31
More WIP
DAreRodz Jun 28, 2024
80c1da9
Restore previous packages
DAreRodz Jun 28, 2024
852bc99
Fix current tests
DAreRodz Jun 28, 2024
0688cff
Add computation tests
DAreRodz Jun 28, 2024
4dec758
Fix getter call
DAreRodz Jun 28, 2024
dace302
Fix for..in on new properties
DAreRodz Jun 28, 2024
20923cb
Refactor code a little
DAreRodz Jun 28, 2024
38c4ca8
Add tests for getters with scope
DAreRodz Jun 28, 2024
98163be
Move namespaces to properties
DAreRodz Jun 28, 2024
55cca00
Fix a problem with object references
DAreRodz Jun 28, 2024
8711d38
Add store handlers
DAreRodz Jun 28, 2024
349a257
Add signals-core dependency
DAreRodz Jun 28, 2024
fc7a39b
Rename Property to PropSignal
DAreRodz Jun 28, 2024
0df6219
Move withScope inside PropSignal logic
DAreRodz Jun 28, 2024
61b1da5
Create proxies folder
DAreRodz Jun 28, 2024
3401a2b
Attempt to make the context work
DAreRodz Jun 28, 2024
5a546eb
Reorganize code
DAreRodz Jun 28, 2024
d375418
Make peek() return only the value inside signalValue
DAreRodz Jun 28, 2024
075c4f9
A bit of refactoring
DAreRodz Jun 28, 2024
b9f5ac8
Return the computed value with `peek()`
DAreRodz Jun 28, 2024
a9a5fb0
Rename DEFAULT_SCOPE to NO_SCOPE
DAreRodz Jun 28, 2024
c9a854c
Remove deepsignal
DAreRodz Jun 28, 2024
fc131dc
Handle functions inside state
DAreRodz Jun 28, 2024
fe0b6bb
Fix withScope in this PR
DAreRodz Jun 28, 2024
e371add
Fix context proxification
DAreRodz Jun 28, 2024
fe1920d
Move store root logic to store proxy handlers
DAreRodz Jun 28, 2024
b471609
Move store initialization inside init
DAreRodz Jun 28, 2024
164d92e
Add TODO comment inside `peek()`
DAreRodz Jun 28, 2024
c06f6bf
Fix store root assignments
DAreRodz Jun 28, 2024
747c822
Fix lint error
DAreRodz Jun 28, 2024
7938dd8
Enable skipped test for non-initialized getters
DAreRodz Jul 1, 2024
c238ede
Rename get(State|Store)Proxy to proxify(State|Store)
DAreRodz Jul 2, 2024
3cf82a1
Make `getContext` throw when there is no scope
DAreRodz Jul 3, 2024
99f6e87
Fix `proxifyState` types
DAreRodz Jul 3, 2024
821532e
Rename some variables in tests
DAreRodz Jul 3, 2024
2dc4548
Add test for object reference keeping
DAreRodz Jul 3, 2024
c720d0f
Rename test suite for state proxy
DAreRodz Jul 3, 2024
8de7b83
Add tests for getters and functions with scope
DAreRodz Jul 3, 2024
4bac1a8
Add tests for prop subscription inside functions
DAreRodz Jul 3, 2024
5e508c1
Allow functions to use `this`
DAreRodz Jul 3, 2024
a4840a0
Minor fixes
DAreRodz Jul 3, 2024
d17f4bd
Add tests to store proxies
DAreRodz Jul 3, 2024
d64eee3
Throw an error when an object cannot be proxified
DAreRodz Jul 4, 2024
22f5244
Change peek implementation
DAreRodz Jul 4, 2024
82622aa
Add tests for peek and unsupported structures
DAreRodz Jul 4, 2024
43dcfec
Test peeking getters that access scope or other namespaces
DAreRodz Jul 4, 2024
e2b2e61
Ignore well-known symbols
DAreRodz Jul 4, 2024
0bd1050
Minor comment format fix
DAreRodz Jul 4, 2024
53c3939
Move namespace arg to first position in proxify functions
DAreRodz Jul 4, 2024
e294bba
chore: Update jest.config.js to stop ignoring deepsignal because we r…
michalczaplinski Jul 9, 2024
c0c633e
Add comments to proxy registry
DAreRodz Jul 16, 2024
1e1aa6a
Remove namespace from PropSignal
DAreRodz Jul 16, 2024
5648754
Remove unused `peekValueSignal` method
DAreRodz Jul 16, 2024
d2f849f
Simplify PropSignal methods
DAreRodz Jul 17, 2024
573ce9d
Add TSDocs to PropSignal
DAreRodz Jul 17, 2024
0950fa8
Disable unused vars lint rule in state-proxy tests
DAreRodz Jul 17, 2024
236e389
Remove descriptor alias
DAreRodz Jul 17, 2024
8bcb576
Expand `getProxyNs` docs
DAreRodz Jul 19, 2024
1b7d2df
Add more comments in `state`
DAreRodz Jul 19, 2024
4554fb4
Refactor state functions and add tsdocs
DAreRodz Jul 23, 2024
5cce756
Fix some grammar issues
DAreRodz Jul 23, 2024
86a5b5a
Fix default namespace for setters
DAreRodz Jul 23, 2024
cebfd6e
Replace `isNotRoot` with `isRoot`
DAreRodz Jul 24, 2024
0e2b095
Update comments in store.ts
DAreRodz Jul 24, 2024
a12732c
Move `isPlainObject` to utils
DAreRodz Jul 24, 2024
0667028
Remove remaining deepSignal references
DAreRodz Jul 24, 2024
9a52b65
Delete unnecessary @ts-ignore-next-line
luisherranz Jul 26, 2024
b21a3ce
Use `isPlainObject` from utils inside store
DAreRodz Jul 29, 2024
e8c5458
Move scopes and namespaces to separate files
DAreRodz Jul 29, 2024
6d1d887
Call `init` outside `DOMContentLoaded`
DAreRodz Jul 29, 2024
4dccb45
Replace `signals-core` imports with `signals`
DAreRodz Jul 30, 2024
fee316d
Rename `proxiedStore` to `proxifiedStore`
DAreRodz Jul 30, 2024
52373b1
Remove unnecessary `peek()` call
DAreRodz Jul 31, 2024
55a9d01
Throw more descriptive errors from getContext and getElement
DAreRodz Jul 31, 2024
ff9c901
Use more descriptive name for proxy functions
DAreRodz Jul 31, 2024
68de3b6
Use destructured `get` inside `setGetter` call
DAreRodz Jul 31, 2024
1009a4f
Add comment to explain `objToIterable` signals subscription
DAreRodz Jul 31, 2024
2e5c926
Change line comment to block comment
DAreRodz Jul 31, 2024
2aaf909
Replace `?` with `!` operator
DAreRodz Jul 31, 2024
1ff465d
Wrap Interactivity API tests with appropriate `describe`
DAreRodz Jul 31, 2024
b470cc2
Remove unnecessary `setNamespace` calls in tests
DAreRodz Jul 31, 2024
65dfdf8
Remove duplicated test
DAreRodz Jul 31, 2024
e0272f8
Fix typo
DAreRodz Jul 31, 2024
4db4bff
Add extra tests for getter modification
DAreRodz Jul 31, 2024
80e9ff5
Test the right namespace is used inside getters
DAreRodz Jul 31, 2024
80cf598
Fix test name
DAreRodz Jul 31, 2024
28d329b
Check if length's PropSignal exists before updating its value
DAreRodz Jul 31, 2024
0efe2c2
Add deepMerge tests and prevent server overwritting
luisherranz Aug 6, 2024
31ad6a9
Make sure context inheritance is shallow and server props don't overw…
luisherranz Aug 6, 2024
185ec72
Refactor wp-each to use new proxifyContext structure
luisherranz Aug 7, 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
32 changes: 0 additions & 32 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -220,14 +220,14 @@
<div
data-wp-interactive="directive-each"
data-wp-router-region="navigation-updated list"
data-wp-context='{ "list": [ "beta", "gamma", "delta" ] }'
data-wp-context='{ "b": 2, "c": 3, "d": 4 }'
data-testid="navigation-updated list"
>
<button
data-testid="navigate"
data-wp-on--click="actions.navigate"
>Navigate</button>
<template data-wp-each="context.list">
<template data-wp-each="state.list">
<p data-wp-text="context.item" data-testid="item"></p>
</template>
<p data-testid="item" data-wp-each-child>beta</p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,14 +169,14 @@ const html = `
<div
data-wp-interactive="directive-each"
data-wp-router-region="navigation-updated list"
data-wp-context='{ "list": [ "alpha", "beta", "gamma", "delta" ] }'
data-wp-context='{ "a": 1, "b": 2, "c": 3, "d": 4 }'
data-testid="navigation-updated list"
>
<button
data-testid="navigate"
data-wp-on--click="actions.navigate"
>Navigate</button>
<template data-wp-each="context.list">
<template data-wp-each="state.list">
<p data-wp-text="context.item" data-testid="item"></p>
</template>
<p data-testid="item" data-wp-each-child>alpha</p>
Expand All @@ -187,6 +187,12 @@ const html = `
`;

store( 'directive-each', {
state: {
get list() {
const ctx = getContext();
return Object.keys( ctx ).sort();
},
},
actions: {
*navigate() {
const { actions } = yield import(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
privateApis,
} from '@wordpress/interactivity';

const { directive, deepSignal, h } = privateApis(
const { directive, proxifyState, h } = privateApis(
'I acknowledge that using private APIs means my theme or plugin will inevitably break in the next version of WordPress.'
);

Expand Down Expand Up @@ -41,12 +41,12 @@ directive(
'test-context',
( { context: { Provider }, props: { children } } ) => {
executionProof( 'context' );
const value = deepSignal( {
[ namespace ]: {
const value = {
[ namespace ]: proxifyState( namespace, {
attribute: 'from context',
text: 'from context',
},
} );
} ),
};
return h( Provider, { value }, children );
},
{ priority: 8 }
Expand Down
8 changes: 4 additions & 4 deletions packages/interactivity-router/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ const {
initialVdom,
toVdom,
render,
parseInitialData,
populateInitialData,
parseServerData,
populateServerData,
batch,
} = privateApis(
'I acknowledge that using private APIs means my theme or plugin will inevitably break in the next version of WordPress.'
Expand Down Expand Up @@ -103,7 +103,7 @@ const regionsToVdom: RegionsToVdom = async ( dom, { vdom } = {} ) => {
} );
}
const title = dom.querySelector( 'title' )?.innerText;
const initialData = parseInitialData( dom );
const initialData = parseServerData( dom );
return { regions, head, title, initialData };
};

Expand All @@ -119,7 +119,7 @@ const renderRegions = ( page: Page ) => {
}
}
if ( navigationMode === 'regionBased' ) {
populateInitialData( page.initialData );
populateServerData( page.initialData );
const attrName = `data-${ directivePrefix }-router-region`;
document
.querySelectorAll( `[${ attrName }]` )
Expand Down
1 change: 0 additions & 1 deletion packages/interactivity/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
"types": "build-types",
"dependencies": {
"@preact/signals": "^1.2.2",
"deepsignal": "^1.4.0",
"preact": "^10.19.3"
},
"publishConfig": {
Expand Down
75 changes: 45 additions & 30 deletions packages/interactivity/src/directives.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,24 @@
*/
import { h as createElement, type RefObject } from 'preact';
import { useContext, useMemo, useRef } from 'preact/hooks';
import { deepSignal, peek, type DeepSignal } from 'deepsignal';
/**
* Internal dependencies
*/
import { proxifyState, peek } from './proxies';

/**
* Internal dependencies
*/
import { useWatch, useInit, kebabToCamelCase, warn, splitTask } from './utils';
import type { DirectiveEntry } from './hooks';
import { directive, getScope, getEvaluate } from './hooks';
import {
useWatch,
useInit,
kebabToCamelCase,
warn,
splitTask,
isPlainObject,
} from './utils';
import { directive, getEvaluate, type DirectiveEntry } from './hooks';
import { getScope } from './scopes';

// Assigned objects should be ignored during proxification.
const contextAssignedObjects = new WeakMap();
Expand All @@ -23,9 +33,6 @@ const contextObjectToProxy = new WeakMap();
const contextProxyToObject = new WeakMap();
const contextObjectToFallback = new WeakMap();

const isPlainObject = ( item: unknown ): boolean =>
Boolean( item && typeof item === 'object' && item.constructor === Object );

const descriptor = Reflect.getOwnPropertyDescriptor;

/**
Expand All @@ -47,7 +54,7 @@ const proxifyContext = ( current: object, inherited: object = {} ): object => {
contextObjectToFallback.set( current, inherited );
if ( ! contextObjectToProxy.has( current ) ) {
const proxy = new Proxy( current, {
get: ( target: DeepSignal< any >, k ) => {
get: ( target: object, k: string ) => {
const fallback = contextObjectToFallback.get( current );
// Always subscribe to prop changes in the current context.
const currentProp = target[ k ];
Expand All @@ -61,9 +68,9 @@ const proxifyContext = ( current: object, inherited: object = {} ): object => {
if (
k in target &&
! contextAssignedObjects.get( target )?.has( k ) &&
isPlainObject( peek( target, k ) )
isPlainObject( currentProp )
) {
return proxifyContext( currentProp, fallback[ k ] );
return proxifyContext( currentProp );
}

// Return the stored proxy for `currentProp` when it exists.
Expand Down Expand Up @@ -125,22 +132,19 @@ const proxifyContext = ( current: object, inherited: object = {} ): object => {
};

/**
* Recursively update values within a deepSignal object.
* Recursively update values within a context object.
*
* @param target A deepSignal instance.
* @param target A context instance.
* @param source Object with properties to update in `target`.
*/
const updateSignals = (
target: DeepSignal< any >,
source: DeepSignal< any >
) => {
const updateContext = ( target: any, source: any ) => {
for ( const k in source ) {
if (
isPlainObject( peek( target, k ) ) &&
isPlainObject( peek( source, k ) )
isPlainObject( source[ k ] )
) {
updateSignals( target[ `$${ k }` ].peek(), source[ k ] );
} else {
updateContext( peek( target, k ) as object, source[ k ] );
} else if ( ! ( k in target ) ) {
target[ k ] = source[ k ];
}
}
Expand Down Expand Up @@ -257,18 +261,21 @@ export default () => {
// data-wp-context
directive(
'context',
// @ts-ignore-next-line
( {
directives: { context },
props: { children },
context: inheritedContext,
} ) => {
const { Provider } = inheritedContext;
const inheritedValue = useContext( inheritedContext );
const currentValue = useRef( deepSignal( {} ) );
const defaultEntry = context.find(
( { suffix } ) => suffix === 'default'
);
const inheritedValue = useContext( inheritedContext );

const ns = defaultEntry!.namespace;
const currentValue = useRef( {
[ ns ]: proxifyState( ns, {} ),
} );

// No change should be made if `defaultEntry` does not exist.
const contextStack = useMemo( () => {
Expand All @@ -280,11 +287,16 @@ export default () => {
`The value of data-wp-context in "${ namespace }" store must be a valid stringified JSON object.`
);
}
updateSignals( currentValue.current, {
[ namespace ]: deepClone( value ),
} );
updateContext(
currentValue.current[ namespace ],
deepClone( value ) as object
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to keep doing a deep clone of the object? If I'm not mistaken, objects are no longer merged on each context, so there is only a unique object for each context. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm, if I remember correctly, I added that because the parsed JSON defined in the data-wp-context directive―i.e., the object returned by toVdom() as a prop―was being modified and that caused some sort of trouble during client-side navigation.

Maybe that won't be needed once we fix #63864. 🤔

Copy link
Member

@luisherranz luisherranz Aug 1, 2024

Choose a reason for hiding this comment

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

the object returned by toVdom() as a prop―was being modified

Hmm... do you know by who? toVdom creates new objects on each run, doesn't it?

const parsedValue = JSON.parse( value );

);
currentValue.current[ namespace ] = proxifyContext(
currentValue.current[ namespace ],
inheritedValue[ namespace ]
);
}
return proxifyContext( currentValue.current, inheritedValue );
return currentValue.current;
}, [ defaultEntry, inheritedValue ] );

return createElement( Provider, { value: contextStack }, children );
Expand Down Expand Up @@ -677,11 +689,14 @@ export default () => {
return list.map( ( item ) => {
const itemProp =
suffix === 'default' ? 'item' : kebabToCamelCase( suffix );
const itemContext = deepSignal( { [ namespace ]: {} } );
const mergedContext = proxifyContext(
itemContext,
inheritedValue
const itemContext = proxifyContext(
proxifyState( namespace, {} ),
inheritedValue[ namespace ]
);
const mergedContext = {
...inheritedValue,
[ namespace ]: itemContext,
};

// Set the item after proxifying the context.
mergedContext[ namespace ][ itemProp ] = item;
Expand Down
Loading
Loading