Interactivity API: add wp-run directive and useInit & useWatch hooks
#57805
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DAreRodz
added
[Type] Feature
DAreRodz
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad and
ajitbohra
as code owners
|
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
|
47 tasks
luisherranz
reviewed
Jan 12, 2024
There was a problem hiding this comment.
This is lovely!!
I think we maybe want to expose a version of
useEffect,useLayoutEffect, etc. (all other basic hooks) that useswithScopeinternally 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.
Why runs instead of callbacks?
There was a problem hiding this comment.
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.
Yeah, I think that we picked callbacks to be able to put everything in that bucket. runs sounds weird to me 😄
| }, | ||
| callbacks: { | ||
| updateIsHydrated() { | ||
| setTimeout( () => ( state.isHydrated = 'yes' ) ); |
There was a problem hiding this comment.
What's with the setTimeouts here?
There was a problem hiding this comment.
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.
Shouldn't we review that infinite loop issue?
There was a problem hiding this comment.
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.
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.
})
); |
luisherranz
approved these changes
Jan 12, 2024
There was a problem hiding this comment.
LGTM! 🎉
Approved, although I'd like to know why you're getting an infinite loop here.
luisherranz
requested changes
Jan 12, 2024
Yup, it was unexpected. I'll investigate that. 🕵️ |
luisherranz
requested changes
Jan 13, 2024
Done in 85b6948. |
luisherranz
approved these changes
Jan 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tracking issue: #56803
Related discussion: #51863
Implements the
data-wp-rundirective 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, theuseInitanduseWatchhooks 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-rundirective was pretty straightforward to implement: it simply evaluates the passed callback.Regarding the
useInitanduseWatch, I had to create a utility function to properly handle the element scope to make it available inside the callback logic.