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: add wp-run directive and useInit & useWatch hooks #57805

Merged
merged 15 commits into from
Jan 15, 2024
Merged
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"$schema": "https://schemas.wp.org/trunk/block.json",
"apiVersion": 2,
"name": "test/directive-run",
"title": "E2E Interactivity tests - directive run",
"category": "text",
"icon": "heart",
"description": "",
"supports": {
"interactivity": true
},
"textdomain": "e2e-interactivity",
"viewScript": "directive-run-view",
"render": "file:./render.php"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php
/**
* HTML for testing the directive `data-wp-run`.
*
* @package gutenberg-test-interactive-blocks
*/

gutenberg_enqueue_module( 'directive-run-view' );
?>

<div
data-wp-interactive='{ "namespace": "directive-run" }'
data-wp-navigation-id='test-directive-run'
>
<div data-testid="hydrated" data-wp-text="state.isHydrated"></div>
<div data-testid="mounted" data-wp-text="state.isMounted"></div>
<div data-testid="renderCount" data-wp-text="state.renderCount"></div>
<div data-testid="navigated">no</div>

<div
data-wp-run--hydrated="callbacks.updateIsHydrated"
data-wp-run--renderCount="callbacks.updateRenderCount"
data-wp-text="state.clickCount"
></div>
</div>

<div data-wp-interactive='{ "namespace": "directive-run" }' >
<button data-testid="toggle" data-wp-on--click="actions.toggle">
Toggle
</button>

<button data-testid="increment" data-wp-on--click="actions.increment">
Increment
</button>

<button data-testid="navigate" data-wp-on--click="actions.navigate">
Navigate
</button>

<!-- Hook execution results are stored in this element as attributes. -->
<div
data-testid="wp-run hooks results"
data-wp-show-children="state.isOpen"
data-init=""
data-watch=""
>
<div
data-wp-run--mounted="callbacks.updateIsMounted"
data-wp-run--hooks="runs.useHooks"
>
Element with wp-run using hooks
</div>
</div>
</div>
110 changes: 110 additions & 0 deletions packages/e2e-tests/plugins/interactive-blocks/directive-run/view.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/**
* WordPress dependencies
*/
import {
store,
directive,
navigate,
useInit,
useWatch,
cloneElement,
getElement,
} from '@wordpress/interactivity';

// Custom directive to show hide the content elements in which it is placed.
directive(
'show-children',
( { directives: { 'show-children': showChildren }, element, evaluate } ) => {
const entry = showChildren.find(
( { suffix } ) => suffix === 'default'
);
return evaluate( entry )
? element
: cloneElement( element, { children: null } );
},
{ priority: 9 }
);

const html = `
<div
data-wp-interactive='{ "namespace": "directive-run" }'
data-wp-navigation-id='test-directive-run'
>
<div data-testid="hydrated" data-wp-text="state.isHydrated"></div>
<div data-testid="mounted" data-wp-text="state.isMounted"></div>
<div data-testid="renderCount" data-wp-text="state.renderCount"></div>
<div data-testid="navigated">yes</div>

<div
data-wp-run--hydrated="callbacks.updateIsHydrated"
data-wp-run--renderCount="callbacks.updateRenderCount"
data-wp-text="state.clickCount"
></div>
</div>
`;

const { state } = store( 'directive-run', {
state: {
isOpen: false,
isHydrated: 'no',
isMounted: 'no',
renderCount: 0,
clickCount: 0
},
actions: {
toggle() {
state.isOpen = ! state.isOpen;
},
increment() {
state.clickCount = state.clickCount + 1;
},
navigate() {
navigate( window.location, {
force: true,
html,
} );
},
},
callbacks: {
updateIsHydrated() {
setTimeout( () => ( state.isHydrated = 'yes' ) );
Copy link
Member

Choose a reason for hiding this comment

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

What's with the setTimeouts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this is weird. I saw an infinite loop issue, at least with state.renderCount (line 76), so I eventually decided to use setTimeout for all callbacks.

PS: I don't know, but regarding this comment, maybe these callbacks should have been runs (or the other way around 🤔).

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we review that infinite loop issue?

Copy link
Member

Choose a reason for hiding this comment

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

Any luck with this?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait. Isn't this just a "don't use setState inside the body function" kind of infinite loop?

You can't do this:

const Comp = () => {
  const [state, setState] = useState(1);
  setState(state + 1); // infinite loop
};

You have to do this, which is equivalent to the setTimeout:

const Comp = () => {
  const [state, setState] = useState(1);
  useEffect(() => {
    setState(state + 1); // still bad, but better :D
  });
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't had a chance to investigate yet, but probably it is what you mention. I'll check tomorrow. 😄

},
updateIsMounted() {
setTimeout( () => ( state.isMounted = 'yes' ) );
},
updateRenderCount() {
setTimeout( () => ( state.renderCount = state.renderCount + 1 ) );
},
},
runs: {
Copy link
Member

Choose a reason for hiding this comment

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

Why runs instead of callbacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was first inside callbacks, but I felt it wasn't the right place for this, which seems more intended for hook callbacks, event listener callbacks, etc. I would use callbacks primarily for reactions or side effects in response to events rather than for arbitrary logic that runs inside an element's render.

Again, this is my subjective impression, so if you think it's better to move them back to callbacks, it's fine. 🙂

(Everything is a callback, isn't it? 🤷)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that we picked callbacks to be able to put everything in that bucket. runs sounds weird to me 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 3ec3afe.

useHooks() {
// Runs only on first render.
useInit( () => {
const { ref } = getElement();
ref
.closest( '[data-testid="wp-run hooks results"]')
.setAttribute( 'data-init', 'initialized' );
return () => {
ref
.closest( '[data-testid="wp-run hooks results"]')
.setAttribute( 'data-init', 'cleaned up' );
};
} );

// Runs whenever a signal consumed inside updates its value. Also
// executes for the first render.
useWatch( () => {
const { ref } = getElement();
const { clickCount } = state;
ref
.closest( '[data-testid="wp-run hooks results"]')
.setAttribute( 'data-watch', clickCount );
return () => {
ref
.closest( '[data-testid="wp-run hooks results"]')
.setAttribute( 'data-watch', 'cleaned up' );
};
} );
}
}
} );
4 changes: 4 additions & 0 deletions packages/interactivity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### New Features

- Add the `data-wp-run` directive along with the `useInit` and `useWatch` hooks. ([57805](https://github.com/WordPress/gutenberg/pull/57805))

### Bug Fix

- Fix namespaces when there are nested interactive regions. ([#57029](https://github.com/WordPress/gutenberg/pull/57029))
Expand Down
23 changes: 14 additions & 9 deletions packages/interactivity/src/directives.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { deepSignal, peek } from 'deepsignal';
* Internal dependencies
*/
import { createPortal } from './portals';
import { useSignalEffect } from './utils';
import { useWatch, useInit } from './utils';
import { directive } from './hooks';
import { SlotProvider, Slot, Fill } from './slots';
import { navigate } from './router';
Expand Down Expand Up @@ -75,14 +75,14 @@ export default () => {
// data-wp-watch--[name]
directive( 'watch', ( { directives: { watch }, evaluate } ) => {
watch.forEach( ( entry ) => {
useSignalEffect( () => evaluate( entry ) );
useWatch( () => evaluate( entry ) );
} );
} );

// data-wp-init--[name]
directive( 'init', ( { directives: { init }, evaluate } ) => {
init.forEach( ( entry ) => {
useEffect( () => evaluate( entry ), [] );
useInit( () => evaluate( entry ) );
} );
} );

Expand Down Expand Up @@ -118,7 +118,7 @@ export default () => {
? `${ currentClass } ${ name }`
: name;

useEffect( () => {
useInit( () => {
// This seems necessary because Preact doesn't change the class
// names on the hydration, so we have to do it manually. It doesn't
// need deps because it only needs to do it the first time.
Expand All @@ -127,7 +127,7 @@ export default () => {
} else {
element.ref.current.classList.add( name );
}
}, [] );
} );
} );
}
);
Expand Down Expand Up @@ -182,7 +182,7 @@ export default () => {
if ( ! result ) delete element.props.style[ key ];
else element.props.style[ key ] = result;

useEffect( () => {
useInit( () => {
// This seems necessary because Preact doesn't change the styles on
// the hydration, so we have to do it manually. It doesn't need deps
// because it only needs to do it the first time.
Expand All @@ -191,7 +191,7 @@ export default () => {
} else {
element.ref.current.style[ key ] = result;
}
}, [] );
} );
} );
} );

Expand All @@ -217,7 +217,7 @@ export default () => {
// This seems necessary because Preact doesn't change the attributes
// on the hydration, so we have to do it manually. It doesn't need
// deps because it only needs to do it the first time.
useEffect( () => {
useInit( () => {
const el = element.ref.current;

// We set the value directly to the corresponding
Expand Down Expand Up @@ -260,7 +260,7 @@ export default () => {
} else {
el.removeAttribute( attribute );
}
}, [] );
} );
}
);
} );
Expand Down Expand Up @@ -390,4 +390,9 @@ export default () => {
),
{ priority: 4 }
);

// data-wp-run
directive( 'run', ( { directives: { run }, evaluate } ) => {
run.forEach( ( entry ) => evaluate( entry ) );
} );
};
13 changes: 11 additions & 2 deletions packages/interactivity/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,17 @@ import { init } from './router';
export { store } from './store';
export { directive, getContext, getElement } from './hooks';
export { navigate, prefetch } from './router';
export { h as createElement } from 'preact';
export { useEffect, useContext, useMemo } from 'preact/hooks';
export {
useWatch,
useInit,
useEffect,
useLayoutEffect,
useCallback,
useMemo,
luisherranz marked this conversation as resolved.
Show resolved Hide resolved
} from './utils';

export { h as createElement, cloneElement } from 'preact';
export { useContext } from 'preact/hooks';
export { deepSignal } from 'deepsignal';

document.addEventListener( 'DOMContentLoaded', async () => {
Expand Down
Loading
Loading