Skip to content

Conversation

@shiba-codes
Copy link

No description provided.

@ZeeCoder
Copy link
Owner

I don't think attaching extra props to native objects like an HTMLElement is a good idea.
I was thinking more of creating some sort of a registry to keep track of observed elements with a WeakMap or something like that. 🤔

@shiba-codes
Copy link
Author

Keeping track of observed elements seems a bit redundant, as ResizeObserver already does just that?
As for an attached method, elements might get a prop with this name in the future, prefixing it like ‘_handleResize’ should help. What other concerns do you have?

@ZeeCoder
Copy link
Owner

Just doesn't feel right to mutate native objects.
Feels similar to adding stuff to global / window.

@hadeeb
Copy link

hadeeb commented Apr 10, 2020

What do you think about dispatching a custom event on the element and adding a listener for it in the hook?

@ZeeCoder
Copy link
Owner

Honestly I'm just not convinced this would worth doing at this point, regardless of the solution.
On the whole internet I've only seen that single comment I've linked to that argues it's more performant having a single instance.
Non of the more popular measuring libraries utilising ResizeObservers use a single instance either, and I see no issues anywhere related to performance.

Anyway, here are some thoughts:

  • I cannot in good conscience just attach callbacks to DOM elements. Even if it would technically work, it would go against best practices about mutating global state, and potential name collisions.
  • I'm not sure how firing custom events would affect performance, and whether that would negate some of the gains of having a single observer. I think the whole reason observers have this API instead of having events fired is for performance reasons. (This solution is bit nicer though in the sense that it doesn't mutate the elements, and I can always just generate a unique event name.)
  • My idea of having a registry (probably a WeakSet) of elements => callbacks is not something I'd actually implement either, as that would mean you now have to maintain an internal registry. Memory consumption might be an issue as well. 🤷‍♂️

All in all at the current state I think it's not worth the effort.

Maybe if someone made performance benchmarks where it's clear that this would become a real-life bottleneck I'd consider it, but at the moment it feels like way too much work for way too little.

@ZeeCoder
Copy link
Owner

I thought I solved this for a second.

I could add a callbacks array, and a single RO that simply calls all callbacks within this array, when changes are detected.
Then all the hook instances need to do is add / remove callbacks as needed.

However within the callback the hooks add, it needs to find the entry that's relevant for the given hook, based on the element observed.

This can be done by using a for loop over the entries array, until target === observed element.

This means that as more hooks (and therefore more observed elements) are added, the entries array might grow in size as well.

☝ This might be something worth looking into though, might not be such a big issue.

Only if a LOT of elements change within the same tick, which would mean that all of these hook-added callbacks would need to loop through the entries array to find only the relevant entry for themselves.

Again, these are the sort of things that really need benchmarking. 😅

@ZeeCoder
Copy link
Owner

See #24

@ZeeCoder ZeeCoder closed this Oct 11, 2020
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 this pull request may close these issues.

3 participants