Image block: fix resize listener #22277
Conversation
ellatrix
requested review from
ajitbohra,
jorgefilipecosta,
Soean,
talldan and
youknowriad
as code owners
ellatrix
added
[Type] Bug
|
Size Change: +4.69 kB (0%) Total Size: 829 kB
ℹ️ View Unchanged
|
| * @param {Array} args `addEventListener` arguments. | ||
| * @param {Array} dependencies Hook dependencies. | ||
| */ | ||
| export default function useGlobalEvent( ref, args, dependencies ) { |
There was a problem hiding this comment.
One of the reasons for the existence of withGlobalEvent is the performance impact of using a single event listener for multiple components using the same args. Do we want to support that here?
There was a problem hiding this comment.
One of the reasons for the existence of withGlobalEvent is the performance impact of using a single event listener for multiple components using the same args. Do we want to support that here?
I forget where exactly I'd remarked about it, but over time I started to question whether there really should expect to be any performance benefit of using one event handler for all global events, since in either case something would need to iterate and invoke each of the handlers. It seems browser native code could likely be more efficient about this.
There was a problem hiding this comment.
I forget where exactly I'd remarked about it
See #18816 (comment)
There was a problem hiding this comment.
To be honest, I find the usage to be a bit awkward, between needing to pass a ref of the node and arguments of addEventListener as an array. ref feels odd because if this is intended to be capturing global events, why should I need to be tracking and passing some reference to my component's local element? (I understand the need, just expressing a sentiment which we could expect from developers using this)
I might wonder instead if...
- We just assume
windowglobal, don't do anything dealing withownerDocument - Or, we just avoid a common hook, and leave it to the individual component to implement. Compared to
withGlobalEvent, there's an appeal here in that (a) it's a bit more straight-forward with hooks and (b) we're not leveraging the "one event handler" behavior ofwithGlobalEventanyways. - Or, change the behavior of the hook to be more of a generic event-handling hook, where we give it the element we want to bind to (give it
ref.current.ownerDocument, orwindow).
For arguments, could it be something like...
useGlobalEvent( 'resize', callback, deps );?
It misses the useCapture argument, which we could either be fine with not supporting, or because it's of a different type than deps, maybe we can overload to support it as a third argument.
useGlobalEvent( 'resize', callback[, useCapture], deps );
|
If we introduce a hook like this, should we refactor |
|
I think it makes sense to let components do it for itself right now and we can decide later if we want a hook for it. To be honest, it might just be better like that because the hooks also doesn't provide an option to set it to |
|
Seeing the code without |
|
|
||
| defaultView.addEventListener( 'resize', calculateSize ); | ||
| image.addEventListener( 'load', calculateSize ); |
There was a problem hiding this comment.
I guess there's no meaningful difference between addEventListener( 'load', fn ) and onload = fn ? (Aside from the ability to have multiple event handlers in the former, which doesn't apply here anyways)
There was a problem hiding this comment.
Exactly. I like the consistency and it's clearer how the listener is removed.
|
Separately worth considering:
|
|
Thanks! Yeah I agree we should warn when using the HoC once we've removed all uses from our own code. |
Description
In #22262 I forgot to account for the
withGlobalEventsHoC. This PR creates a newuseGlobalEventhook to provide the same functionality for function components.How has this been tested?
Resizing an image shouldn't log errors.
Screenshots
Types of changes
Checklist: