-
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: add wp-run
directive and useInit
& useWatch
hooks
#57805
Conversation
I think we maybe want to expose a version of Let me know what you think. 🙂 cc: @luisherranz, @c4rl0sbr4v0 |
Size Change: +902 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
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.
This is lovely!!
I think we maybe want to expose a version of
useEffect
,useLayoutEffect
, etc. (all other basic hooks) that useswithScope
internally to makegetElement()
andgetContext()
safe to use in any hook callback.Let me know what you think. 🙂
Yes! Let's do that. I think we can do it in this PR as well.
setTimeout( () => ( state.renderCount = state.renderCount + 1 ) ); | ||
}, | ||
}, | ||
runs: { |
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.
Why runs
instead of callbacks
?
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 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? 🤷)
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.
Yeah, I think that we picked callbacks
to be able to put everything in that bucket. runs
sounds weird to me 😄
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.
Fixed in 3ec3afe.
}, | ||
callbacks: { | ||
updateIsHydrated() { | ||
setTimeout( () => ( state.isHydrated = 'yes' ) ); |
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.
What's with the setTimeout
s here?
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.
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 🤔).
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.
Shouldn't we review that infinite loop issue?
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.
Any luck with this?
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.
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
});
};
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.
I haven't had a chance to investigate yet, but probably it is what you mention. I'll check tomorrow. 😄
Preventing "why is this not working" is always a good idea. 🚀 While exposing these, we need to make sure to document they are not the same as the ones inside |
Webpack is going to throw if people try to use |
Note: if we finally expose window.addEventListener(
"scroll",
withScope(function* () {
// ...
yield somethingAsync();
// scope recovered here.
})
); |
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.
LGTM! 🎉
Approved, although I'd like to know why you're getting an infinite loop here.
Yup, it was unexpected. I'll investigate 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.
Oh, sorry. Can you add some basic docs for data-wp-run
?
Done in 85b6948. |
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.
I've added some minor modifications to the docs, and we should also reexport useState
and useRef
, but after than, we can merge.
Tracking issue: #56803
Related discussion: #51863
Implements the
data-wp-run
directive to allow developers to define their custom logic to execute while rendering an element with directives. That logic can also include any hook to be executed.Along with
data-wp-run
, theuseInit
anduseWatch
hooks are implemented.Why?
These features are required for the future public version of the Interactivity API. See the tracking issue for details.
How?
The
data-wp-run
directive was pretty straightforward to implement: it simply evaluates the passed callback.Regarding the
useInit
anduseWatch
, I had to create a utility function to properly handle the element scope to make it available inside the callback logic.