Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Add a useSignalEffect implementation that behaves like useEffect #226

Merged
merged 11 commits into from
May 4, 2023

Conversation

DAreRodz
Copy link
Collaborator

The current useSignalEffect hook implementation from @preact/signals calls the passed callback before changes to the DOM are made, but it should wait for DOM changes (like useEffect does).

I just copy-pasted the useSignalEffect hook and its dependencies from preactjs/signals#290 until those changes are merged and released in @preact/signals.

@DAreRodz DAreRodz requested a review from luisherranz April 21, 2023 11:32
@luisherranz
Copy link
Member

I've been trying to add tests to reproduce the problem here and to reproduce that I needed a newer version of wp-show that actually removes the elements from the DOM. So I added it, but then the tests of wp-show where not passing because the new createTreeWalker algorithm doesn't support templates, so I started adding support for that. That part of the code is still a mess and I'm not even sure if it works, so I'll keep working on that on Monday, or I'll remove the code and I'll skip the test until we add this in another PR.

The good news is that the new useSignalEffect passes the tests and the older one doesn't so it's an improvement 🙂

@luisherranz
Copy link
Member

I couldn't use data-wp-bind.hidden because then the test that checks that effects are removed when the DOM element is removed fails, but it turns out that the tests pass with the current implementation of wp-show as well.

This should be ready now. As you started the PR, I've preapproved it. Feel free to review and merge 🙂

@luisherranz
Copy link
Member

Oh, the tests pass on my computer but not here. I don't know what's going on.

@luisherranz
Copy link
Member

Well, I ended up:

  • Adding a temporary fake wp-show directive that removes things from the DOM because the other was failing in webkit.
  • Testing if something was added to the DOM with state and an element instead of checking a value in window because it was a bit flaky.

Things should be ready now.

Copy link
Collaborator Author

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

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

Thanks @luisherranz! The changes you did look great to me. 😄

@@ -58,7 +58,7 @@ export default () => {
const contextValue = useContext(context);
Object.entries(on).forEach(([name, path]) => {
element.props[`on${name}`] = (event) => {
evaluate(path, { event, context: contextValue });
return evaluate(path, { event, context: contextValue });
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This return is not needed; I'm pretty sure event listener callbacks always return undefined (see this). I'll change this, don't worry. 🙂

Copy link
Member

@luisherranz luisherranz May 15, 2023

Choose a reason for hiding this comment

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

Oh, I see. So instead of returning false in the onclick attribute, you'd use event.stopPropagation() in the event listener. And Preact is using the listener, not the attribute, right?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants