From c696f2e30aea5c944580ae95340483d5638d8f69 Mon Sep 17 00:00:00 2001 From: Luis Herranz Date: Wed, 5 Jul 2023 12:12:09 +0200 Subject: [PATCH 1/6] Remove the Directive component Co-authored-by: David Arenas --- packages/interactivity/src/hooks.js | 67 ++++++++++++----------------- packages/interactivity/src/vdom.js | 5 ++- 2 files changed, 31 insertions(+), 41 deletions(-) diff --git a/packages/interactivity/src/hooks.js b/packages/interactivity/src/hooks.js index e309990482ebc2..d7684644418769 100644 --- a/packages/interactivity/src/hooks.js +++ b/packages/interactivity/src/hooks.js @@ -1,8 +1,7 @@ /** * External dependencies */ -import { h, options, createContext, cloneElement } from 'preact'; -import { useRef, useMemo } from 'preact/hooks'; +import { options, createContext, cloneElement } from 'preact'; /** * Internal dependencies */ @@ -47,40 +46,21 @@ const getEvaluate = // Separate directives by priority. The resulting array contains objects // of directives grouped by same priority, and sorted in ascending order. -const usePriorityLevels = ( directives ) => - useMemo( () => { - const byPriority = Object.entries( directives ).reduce( - ( acc, [ name, values ] ) => { - const priority = directivePriorities[ name ]; - if ( ! acc[ priority ] ) acc[ priority ] = {}; - acc[ priority ][ name ] = values; - - return acc; - }, - {} - ); - - return Object.entries( byPriority ) - .sort( ( [ p1 ], [ p2 ] ) => p1 - p2 ) - .map( ( [ , obj ] ) => obj ); - }, [ directives ] ); - -// Directive wrapper. -const Directive = ( { type, directives, props: originalProps } ) => { - const ref = useRef( null ); - const element = h( type, { ...originalProps, ref } ); - const evaluate = useMemo( () => getEvaluate( { ref } ), [] ); - - // Add wrappers recursively for each priority level. - const byPriorityLevel = usePriorityLevels( directives ); - return ( - +const getPriorityLevels = ( directives ) => { + const byPriority = Object.entries( directives ).reduce( + ( acc, [ name, values ] ) => { + const priority = directivePriorities[ name ]; + if ( ! acc[ priority ] ) acc[ priority ] = {}; + acc[ priority ][ name ] = values; + + return acc; + }, + {} ); + + return Object.entries( byPriority ) + .sort( ( [ p1 ], [ p2 ] ) => p1 - p2 ) + .map( ( [ , obj ] ) => obj ); }; // Priority level wrapper. @@ -131,14 +111,21 @@ const old = options.vnode; options.vnode = ( vnode ) => { if ( vnode.props.__directives ) { const props = vnode.props; - const directives = props.__directives; + const priorityLevels = getPriorityLevels( props.__directives ); delete props.__directives; + const ref = { current: props.__node }; + delete props.__node; vnode.props = { - type: vnode.type, - directives, - props, + directives: priorityLevels, + originalProps: props, + evaluate: getEvaluate( { ref } ), + element: { + type: vnode.type, + ref, + props, + }, }; - vnode.type = Directive; + vnode.type = RecursivePriorityLevel; } if ( old ) old( vnode ); diff --git a/packages/interactivity/src/vdom.js b/packages/interactivity/src/vdom.js index fe09f492dbd566..e823ecff2e2bc9 100644 --- a/packages/interactivity/src/vdom.js +++ b/packages/interactivity/src/vdom.js @@ -92,7 +92,10 @@ export function toVdom( root ) { ]; if ( island ) hydratedIslands.add( node ); - if ( hasDirectives ) props.__directives = directives; + if ( hasDirectives ) { + props.__directives = directives; + props.__node = node; + } let child = treeWalker.firstChild(); if ( child ) { From c840d2d0fec636916c92f5c3612f6929a68efc15 Mon Sep 17 00:00:00 2001 From: Luis Herranz Date: Wed, 19 Jul 2023 14:50:41 +0200 Subject: [PATCH 2/6] Move ref to first Recursive component --- packages/interactivity/src/hooks.js | 27 +++++++++++++++++---------- packages/interactivity/src/vdom.js | 5 +---- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/interactivity/src/hooks.js b/packages/interactivity/src/hooks.js index d7684644418769..b43471953ea050 100644 --- a/packages/interactivity/src/hooks.js +++ b/packages/interactivity/src/hooks.js @@ -1,7 +1,8 @@ /** * External dependencies */ -import { options, createContext, cloneElement } from 'preact'; +import { h, options, createContext, cloneElement } from 'preact'; +import { useRef, useCallback } from 'preact/hooks'; /** * Internal dependencies */ @@ -69,12 +70,23 @@ const RecursivePriorityLevel = ( { element, evaluate, originalProps, + elementRef, } ) => { + if ( ! elementRef ) { + // eslint-disable-next-line react-hooks/rules-of-hooks + elementRef = useRef( null ); + } + + if ( ! evaluate ) { + // eslint-disable-next-line react-hooks/rules-of-hooks + evaluate = useCallback( getEvaluate( { ref: elementRef } ), [] ); + } + // This element needs to be a fresh copy so we are not modifying an already // rendered element with Preact's internal properties initialized. This // prevents an error with changes in `element.props.children` not being // reflected in `element.__k`. - element = cloneElement( element ); + element = cloneElement( element, { ref: elementRef } ); // Recursively render the wrapper for the next priority level. // @@ -90,6 +102,7 @@ const RecursivePriorityLevel = ( { element={ element } evaluate={ evaluate } originalProps={ originalProps } + elementRef={ elementRef } /> ) : ( element @@ -113,17 +126,11 @@ options.vnode = ( vnode ) => { const props = vnode.props; const priorityLevels = getPriorityLevels( props.__directives ); delete props.__directives; - const ref = { current: props.__node }; - delete props.__node; vnode.props = { directives: priorityLevels, originalProps: props, - evaluate: getEvaluate( { ref } ), - element: { - type: vnode.type, - ref, - props, - }, + type: vnode.type, + element: h( vnode.type, props ), }; vnode.type = RecursivePriorityLevel; } diff --git a/packages/interactivity/src/vdom.js b/packages/interactivity/src/vdom.js index e823ecff2e2bc9..fe09f492dbd566 100644 --- a/packages/interactivity/src/vdom.js +++ b/packages/interactivity/src/vdom.js @@ -92,10 +92,7 @@ export function toVdom( root ) { ]; if ( island ) hydratedIslands.add( node ); - if ( hasDirectives ) { - props.__directives = directives; - props.__node = node; - } + if ( hasDirectives ) props.__directives = directives; let child = treeWalker.firstChild(); if ( child ) { From f4f3990da01b397eabc3ee664a7f0b18c7dd2880 Mon Sep 17 00:00:00 2001 From: Luis Herranz Date: Wed, 19 Jul 2023 15:15:11 +0200 Subject: [PATCH 3/6] Pass all the directives to the directive callbacks --- packages/interactivity/src/hooks.js | 54 ++++++++++++----------------- 1 file changed, 22 insertions(+), 32 deletions(-) diff --git a/packages/interactivity/src/hooks.js b/packages/interactivity/src/hooks.js index b43471953ea050..671d27ded3b90b 100644 --- a/packages/interactivity/src/hooks.js +++ b/packages/interactivity/src/hooks.js @@ -65,44 +65,31 @@ const getPriorityLevels = ( directives ) => { }; // Priority level wrapper. -const RecursivePriorityLevel = ( { - directives: [ directives, ...rest ], +const Directives = ( { + directives, + priorityLevels: [ thisPriorityLevel, ...restPriorityLevels ], element, evaluate, originalProps, - elementRef, + elemRef, } ) => { - if ( ! elementRef ) { - // eslint-disable-next-line react-hooks/rules-of-hooks - elementRef = useRef( null ); - } - - if ( ! evaluate ) { - // eslint-disable-next-line react-hooks/rules-of-hooks - evaluate = useCallback( getEvaluate( { ref: elementRef } ), [] ); - } - - // This element needs to be a fresh copy so we are not modifying an already - // rendered element with Preact's internal properties initialized. This - // prevents an error with changes in `element.props.children` not being - // reflected in `element.__k`. - element = cloneElement( element, { ref: elementRef } ); + // eslint-disable-next-line react-hooks/rules-of-hooks + elemRef = elemRef || useRef( null ); + // eslint-disable-next-line react-hooks/rules-of-hooks, react-hooks/exhaustive-deps + evaluate = evaluate || useCallback( getEvaluate( { ref: elemRef } ), [] ); + // Create a fresh copy of the vnode element. + element = cloneElement( element, { ref: elemRef } ); // Recursively render the wrapper for the next priority level. - // - // Note that, even though we're instantiating a vnode with a - // `RecursivePriorityLevel` here, its render function will not be executed - // just yet. Actually, it will be delayed until the current render function - // has finished. That ensures directives in the current priorty level have - // run (and thus modified the passed `element`) before the next level. const children = - rest.length > 0 ? ( - 0 ? ( + ) : ( element @@ -111,7 +98,7 @@ const RecursivePriorityLevel = ( { const props = { ...originalProps, children }; const directiveArgs = { directives, props, element, context, evaluate }; - for ( const d in directives ) { + for ( const d in thisPriorityLevel ) { const wrapper = directiveMap[ d ]?.( directiveArgs ); if ( wrapper !== undefined ) props.children = wrapper; } @@ -124,15 +111,18 @@ const old = options.vnode; options.vnode = ( vnode ) => { if ( vnode.props.__directives ) { const props = vnode.props; - const priorityLevels = getPriorityLevels( props.__directives ); + const directives = props.__directives; delete props.__directives; + const priorityLevels = getPriorityLevels( directives ); vnode.props = { - directives: priorityLevels, + directives, + priorityLevels, originalProps: props, type: vnode.type, element: h( vnode.type, props ), + top: true, }; - vnode.type = RecursivePriorityLevel; + vnode.type = Directives; } if ( old ) old( vnode ); From 03494fb61159a997bccb2f7d81051a259381dab4 Mon Sep 17 00:00:00 2001 From: Luis Herranz Date: Wed, 19 Jul 2023 15:50:06 +0200 Subject: [PATCH 4/6] Don't create components for non-registered directives --- .../directive-priorities/render.php | 4 +++ packages/interactivity/src/hooks.js | 35 ++++++++++--------- .../directive-priorities.spec.ts | 17 +++++++++ 3 files changed, 40 insertions(+), 16 deletions(-) diff --git a/packages/e2e-tests/plugins/interactive-blocks/directive-priorities/render.php b/packages/e2e-tests/plugins/interactive-blocks/directive-priorities/render.php index a5f0b045ab0d67..13e15b8e141714 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/directive-priorities/render.php +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-priorities/render.php @@ -18,3 +18,7 @@ data-wp-test-context > + +
+
+
diff --git a/packages/interactivity/src/hooks.js b/packages/interactivity/src/hooks.js index 671d27ded3b90b..50153850de84b0 100644 --- a/packages/interactivity/src/hooks.js +++ b/packages/interactivity/src/hooks.js @@ -12,10 +12,10 @@ import { rawStore as store } from './store'; const context = createContext( {} ); // WordPress Directives. -const directiveMap = {}; +const directiveCallbacks = {}; const directivePriorities = {}; export const directive = ( name, cb, { priority = 10 } = {} ) => { - directiveMap[ name ] = cb; + directiveCallbacks[ name ] = cb; directivePriorities[ name ] = priority; }; @@ -50,10 +50,11 @@ const getEvaluate = const getPriorityLevels = ( directives ) => { const byPriority = Object.entries( directives ).reduce( ( acc, [ name, values ] ) => { - const priority = directivePriorities[ name ]; - if ( ! acc[ priority ] ) acc[ priority ] = {}; - acc[ priority ][ name ] = values; - + if ( directiveCallbacks[ name ] ) { + const priority = directivePriorities[ name ]; + if ( ! acc[ priority ] ) acc[ priority ] = {}; + acc[ priority ][ name ] = values; + } return acc; }, {} @@ -99,7 +100,7 @@ const Directives = ( { const directiveArgs = { directives, props, element, context, evaluate }; for ( const d in thisPriorityLevel ) { - const wrapper = directiveMap[ d ]?.( directiveArgs ); + const wrapper = directiveCallbacks[ d ]?.( directiveArgs ); if ( wrapper !== undefined ) props.children = wrapper; } @@ -114,15 +115,17 @@ options.vnode = ( vnode ) => { const directives = props.__directives; delete props.__directives; const priorityLevels = getPriorityLevels( directives ); - vnode.props = { - directives, - priorityLevels, - originalProps: props, - type: vnode.type, - element: h( vnode.type, props ), - top: true, - }; - vnode.type = Directives; + if ( priorityLevels.length > 0 ) { + vnode.props = { + directives, + priorityLevels, + originalProps: props, + type: vnode.type, + element: h( vnode.type, props ), + top: true, + }; + vnode.type = Directives; + } } if ( old ) old( vnode ); diff --git a/test/e2e/specs/interactivity/directive-priorities.spec.ts b/test/e2e/specs/interactivity/directive-priorities.spec.ts index 1734d6f5aecc36..56745bfad0c433 100644 --- a/test/e2e/specs/interactivity/directive-priorities.spec.ts +++ b/test/e2e/specs/interactivity/directive-priorities.spec.ts @@ -81,4 +81,21 @@ test.describe( 'Directives (w/ priority)', () => { ].join( ', ' ) ); } ); + + test( 'should not create a Directives component if none of the directives are registered', async ( { + page, + } ) => { + const nonExistentDirectives = page.getByTestId( + 'non-existent-directives' + ); + expect( + await nonExistentDirectives.evaluate( + // This returns undefined if type is a component. + ( node ) => { + return ( node as any ).__k.__k.__k[ 0 ].__k[ 0 ].__k[ 0 ] + .type; + } + ) + ).toBe( 'div' ); + } ); } ); From bac6780f1819296a5c5369102a12adf99daad528 Mon Sep 17 00:00:00 2001 From: Luis Herranz Date: Wed, 19 Jul 2023 16:38:55 +0200 Subject: [PATCH 5/6] Just store directive names in priorityLevels --- packages/interactivity/src/hooks.js | 32 ++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/interactivity/src/hooks.js b/packages/interactivity/src/hooks.js index 50153850de84b0..d1cf122008195f 100644 --- a/packages/interactivity/src/hooks.js +++ b/packages/interactivity/src/hooks.js @@ -14,8 +14,8 @@ const context = createContext( {} ); // WordPress Directives. const directiveCallbacks = {}; const directivePriorities = {}; -export const directive = ( name, cb, { priority = 10 } = {} ) => { - directiveCallbacks[ name ] = cb; +export const directive = ( name, callback, { priority = 10 } = {} ) => { + directiveCallbacks[ name ] = callback; directivePriorities[ name ] = priority; }; @@ -48,21 +48,17 @@ const getEvaluate = // Separate directives by priority. The resulting array contains objects // of directives grouped by same priority, and sorted in ascending order. const getPriorityLevels = ( directives ) => { - const byPriority = Object.entries( directives ).reduce( - ( acc, [ name, values ] ) => { - if ( directiveCallbacks[ name ] ) { - const priority = directivePriorities[ name ]; - if ( ! acc[ priority ] ) acc[ priority ] = {}; - acc[ priority ][ name ] = values; - } - return acc; - }, - {} - ); + const byPriority = Object.keys( directives ).reduce( ( obj, name ) => { + if ( directiveCallbacks[ name ] ) { + const priority = directivePriorities[ name ]; + ( obj[ priority ] = obj[ priority ] || [] ).push( name ); + } + return obj; + }, {} ); return Object.entries( byPriority ) .sort( ( [ p1 ], [ p2 ] ) => p1 - p2 ) - .map( ( [ , obj ] ) => obj ); + .map( ( [ , arr ] ) => arr ); }; // Priority level wrapper. @@ -74,10 +70,14 @@ const Directives = ( { originalProps, elemRef, } ) => { + // Initialize the DOM reference. // eslint-disable-next-line react-hooks/rules-of-hooks elemRef = elemRef || useRef( null ); + + // Create a reference to the evaluate function using the DOM reference. // eslint-disable-next-line react-hooks/rules-of-hooks, react-hooks/exhaustive-deps evaluate = evaluate || useCallback( getEvaluate( { ref: elemRef } ), [] ); + // Create a fresh copy of the vnode element. element = cloneElement( element, { ref: elemRef } ); @@ -99,8 +99,8 @@ const Directives = ( { const props = { ...originalProps, children }; const directiveArgs = { directives, props, element, context, evaluate }; - for ( const d in thisPriorityLevel ) { - const wrapper = directiveCallbacks[ d ]?.( directiveArgs ); + for ( const directiveName of thisPriorityLevel ) { + const wrapper = directiveCallbacks[ directiveName ]?.( directiveArgs ); if ( wrapper !== undefined ) props.children = wrapper; } From 228a94034c34ee2d0f540eb95f79d05c20c441b8 Mon Sep 17 00:00:00 2001 From: Luis Herranz Date: Fri, 21 Jul 2023 13:11:42 +0200 Subject: [PATCH 6/6] Rename internal priority level variables --- packages/interactivity/src/hooks.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/interactivity/src/hooks.js b/packages/interactivity/src/hooks.js index d1cf122008195f..81e2d0713fd0e7 100644 --- a/packages/interactivity/src/hooks.js +++ b/packages/interactivity/src/hooks.js @@ -64,7 +64,7 @@ const getPriorityLevels = ( directives ) => { // Priority level wrapper. const Directives = ( { directives, - priorityLevels: [ thisPriorityLevel, ...restPriorityLevels ], + priorityLevels: [ currentPriorityLevel, ...nextPriorityLevels ], element, evaluate, originalProps, @@ -83,10 +83,10 @@ const Directives = ( { // Recursively render the wrapper for the next priority level. const children = - restPriorityLevels.length > 0 ? ( + nextPriorityLevels.length > 0 ? (