Use single ResizeObserver for better performance

[xjmdoo]

According to the guidelines of using the ResizeObserver API the recommended approach is to use a single observer to observe multiple elements because it provides significant performance gains compared to creating a new observer for each element.

[elwayman02]

Thank you for submitting this PR! I think it's a great idea to move toward a more performant solution if we don't need more than one observer in an app.

I don't think this change quite accomplishes the goal you set out to solve. Since modifiers are not singletons, and there is only ever one handler per modifier, then the code as written (prior to your change) will always have 1 observer per 1 handler per 1 modifier. This PR abuses the class definition as a reference to try and work around that, but I think that paradigm is a bit confusing in regards to the way Ember patterns think about classes and modifiers.

It sounds like what you actually want is a centralized singleton service to manage all ResizeObserver events from a single observer attached to many elements, akin to what we do for IntersectionObservers:

https://github.com/snewcomer/intersection-observer-admin
https://github.com/elwayman02/ember-scroll-modifiers/blob/master/addon/services/observer-manager.js

Does that seem correct or have I misunderstood what you're trying to accomplish?

[xjmdoo]

I don't think this change accomplishes the goal you set out to solve. Since modifiers are not singletons, and there is only ever one handler per modifier, then the code as written will always have 1 observer per 1 handler per 1 modifier. This PR does not change that, as the cached observer still only exists within the context of each separate modifier, which do not share state between them.

It sounds like what you actually want is a centralized singleton service to manage all ResizeObserver events from a single observer attached to many elements, akin to what we do for IntersectionObservers:

https://github.com/snewcomer/intersection-observer-admin
https://github.com/elwayman02/ember-scroll-modifiers/blob/master/addon/services/observer-manager.js

Does that seem correct or have I misunderstood what you're trying to accomplish?

Not sure if I understand your point, static properties on a class are shared between instances, because they are part of the class not the instance. I can easily miss something ember specific here but after testing, it was working and adding multiple modifiers to the page reused the observer and the one callback function managed all the handlers.

[elwayman02]

@xjmdoo Sorry for the lack of clarity in my initial response - it took me a bit to fully process your change and I edited my response above to be a bit more accurate. Generally, I am not sure if static class properties are the right pattern for this. I'm not saying they're not, but it's not something I've given a lot of consideration to. It's a novel approach and I want to noodle on it a bit and get some feedback from others before we move forward. It may well be that what you've done here is the best approach and it just wasn't something that occurred to me, in which case that's great!

I'm also adding this to the PR as a reference for others and justification for why this change is important:

WICG/resize-observer#59 (comment)
https://groups.google.com/a/chromium.org/g/blink-dev/c/z6ienONUb5A/m/F5-VcUZtBAAJ?pli=1

TLDR tests show that sharing a single ResizeObserver can be up to 10x faster than having a 1:1 relationship between elements and ResizeObservers. That's a pretty big win!

[xjmdoo]

@elwayman02 Thank you! Looking forward to your feedback. :)

[elwayman02]

Ok! I'm back after discussing it with a few folks, and while services are a really comfortable approach, we think it's mostly 50/50 with either approach being valid. In this case, there are a couple things we kept in mind:

1. services are a more well-known Ember pattern, but static class properties are a native pattern and thus easier to teach even outside of Ember.
2. services are maybe slightly easier to test, but regardless either approach will require some knowledge of the internal implementation in order to properly test usage as a consumer
3. teardown with your approach is dead simple, as each modifier can simply remove itself when it's being torn down

So ultimately, we're fine moving forward with this approach! Thanks for bearing with me as I worked through that. I'll take a look through the PR again to just review for any issues with the implementation now that we're aligned on using the static class properties!

[elwayman02]

Overall, I think this looks great @xjmdoo! I left comments for a few minor tweaks/improvements we can consider, but this is in great shape and I think it won't take much at all to land this. Thank you for taking on such an important improvement for the addon; I'm glad you're finding it useful enough to contribute back to its development!

[elwayman02]

Closes #214

[xjmdoo]

Thanks for the review @elwayman02 , all valid points. I've updated the PR with the changes and added some improvements to testing. Also, this should fix #46 now.

[elwayman02]

Looks great! One more comment to see if we can write a reasonable test case just to make this a bit more solid. Otherwise, I think it's ready to go!

[elwayman02]

This test looks great! Nice work

[elwayman02]

Confirmed that the demo app still works in the preview, so I think we're good to go! Seeing as this is an enhancement that doesn't change the external API, I think we can release this as a minor version with no breaking changes. Thank you so much for this contribution @xjmdoo!