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

Erratic behaviour for simple example with toggling a link #3596

Closed
1 task
oe-st opened this issue Jun 30, 2022 · 6 comments · Fixed by #3608
Closed
1 task

Erratic behaviour for simple example with toggling a link #3596

oe-st opened this issue Jun 30, 2022 · 6 comments · Fixed by #3608

Comments

@oe-st
Copy link

oe-st commented Jun 30, 2022

  • Check if updating to the latest Preact version resolves the issue

Describe the bug
A very simple link that toggles "itself" with useState behaves erratically. The change happens twice, depending on structure of underlying html. See below (or also https://stackoverflow.com/questions/72803084/simple-example-with-preact-is-firing-twice-on-usestate). I am using v10.4.6.

It's a bit far fetched that this has not been discovered before, so I suspect there is something going on here that is not a bug in Preact itself. But I cannot think if what it might be.

To Reproduce

Notice the difference in the html for various cases below. They should all work the same (unless there is something about Preact I don't understand yet).

The following does not work. The text does not toggle, but console log shows it toggle back and forth for each click.
https://codesandbox.io/s/compassionatwwwe-bogdan-2h5rko-2h5rko?file=/index.js

The following also does not work, but behaves differently. The first click works as expected and subsequent once do not.
https://codesandbox.io/s/compassionatwww1e-bogdan-2h5rko-forked-dzheqi?file=/index.js

The following works as expected:
https://codesandbox.io/s/compassionatwww1e-bogdan-2h5rko-forked-njtlv6?file=/index.js

Expected behavior
All three examples should properly toggle the text for each click.

@JoviDeCroock
Copy link
Member

I agree with you that this feels buggy, generally however this is a quirk in the diffing. We are introducing different levels in diffing and it seems to not carry over the DOM correctly. Will look into whether we can fix this.

There are a few solutions around this luckily:

  • if the dom switches structure within the same functional vnode you can levereage key to convey this information
  • use a button for the clickable thing solves all the cases

@oe-st
Copy link
Author

oe-st commented Jun 30, 2022

Interesting, I actually tried with adding id to the items, but I should ofc have tried with key since that is what is internally used. I added keys for the divs and it started to work.

Seems a bit scary, so maybe at least add a warning if items don't have keys?

Huge thanks for your swift response!

@marvinhagemeister
Copy link
Member

It looks like we're rendering too quickly, before the event is fully dispatched. When the event bubbles up to the outer <div> we've already updated that with the new click listener and than that is getting fired.

@JoviDeCroock
Copy link
Member

Peculiar thing is that if I call e.stopPropagation() https://codesandbox.io/s/compassionatwww1e-bogdan-2h5rko-forked-b88d4v it also works, maybe a microtick isn't enough of a wait for the DOM?

@developit
Copy link
Member

FWIW the reason this happens is because we batch state updates using a microtask (Promise.then()). The microtask queue is flushed between each event listener invocation, which means we execute the render triggered by the state update before the click event bubbles up to its parent node.

This was the main reason I'd considered advocating for us switching debounceRendering back to setTimeout a while back. It has other tradeoffs, but it would mean that we'd never render during event handling. Here's the first demo with options.debounceRendering=setTimeout, which works as expected:
https://codesandbox.io/s/compassionatwww1e-bogdan-2h5rko-forked-vlh5p1?file=/index.js:84-124

@JoviDeCroock
Copy link
Member

Fixed in #3608

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

Successfully merging a pull request may close this issue.

4 participants