-
Notifications
You must be signed in to change notification settings - Fork 47k
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
putListener
dominating ReactPerf.printDOM() and printWasted()
#5189
Comments
Just to add some context, whether or not I use actual anchor tags doesn't seem to affect whether or not putListener gets called. |
@STRML Can you provide a simplified jsfiddle which demonstrates the issue (too many putlisteners) so we can take a look. |
Yea, this looks like the workaround for removing the tap delay. cc @spicyj |
I'll work on a jsfiddle when I get home tonight - thanks for taking a look. The perf is a concern to me but I've been seeing crashes / error logs in Sentry due to |
Got it: https://jsfiddle.net/69z2wepo/18425/ - open the console and hit Run. |
The |
This all appears to be expected behavior to me. When you pass a different function instance that counts as a change. Maybe we should ignore event listeners? Those updates are usually less costly and are often no-ops. |
Yeah, if interacting with the DOM is slow, we could probably set a callback proxy that, when invoked, reads the actual handler off the internal instance. Then "updates" due to a |
So is it considered bad form to bind in a render function or otherwise create handlers in the loop? The alternative, most of the time, is just to put the data in a |
These already don't actually interact with the DOM. |
If they don't interact with the DOM, then it seems erroneous to have them print in
|
I mean, that method lists every other side effect of returning a new element from |
Ok, TLDR: Conceptually the DOM is changing due to the In practice, the 97% use case is probably binds (rather than someone swapping out an event handler, which would be possible but slightly unusual). In the interest of making ReactPerf "useful" rather than "correct" @spicyj and I are in agreement that it probably makes sense to suppress logging updates to the event handlers if/when the previous value and new value are both functions. |
Fixed in #5209 |
Fix for lint issues Added the test cases for the issue 5189 Removing empty space Removing the Eventplugin instrumentation code Removing unwanted white space
I'm still seeing a bunch of "putListener" events when I call After reading this thread, these are the things I am led to believe:
|
I haven’t dived into how printDOM() or event dispatching works so I can’t say. |
The fix from #5209 was merged into master and not into the 0.14 branch. The calls are still there in 0.14. This is why you’re seeing them. Give 15.0 RC2 a try. It should contain the relevant fix. |
Ah, that makes sense. Thanks. |
I believe this is a Safari hack? I'm seeing
putListener
get out of control, even for elements that haven't updated (they've rerendered, but no change was necessary to the DOM).This also is causing weird interactions with react-raf-batching, which we've forked for 0.13 and 0.14. It was working fine in 0.13 but now, fast interactions cause
findComponentRoot()
errors as it's attempting to runputListener()
.Example printDOM() output:
The text was updated successfully, but these errors were encountered: