react-intersection-observer does not provide all members of IntersectionObserverEntry

[jamesism]

Overview

When using react-intersection-observer only the isIntersecting member of IntersectionObserverEntry is present on the returned intersection from const [intersection] = useIntersection().

This is because all of the members of IntersectionObserverEntry are non-enumerable, and the current implementation tries to spread them across an internal state setter which fails when transpiled to an Object.assign. The isIntersecting property is still present as it is set as a guard for browser support.

I believe these properties are implemented as getters (or so it seems..) in order to be lazily loaded and avoid the performance hit of calculating bounding boxes unless the specific property is actually requested in user land. I have tried a solution that would copy that implementation directly (using defineProperties and getOwnPropertyDescriptors), but oddly enough the members of IntersectionObserverEntry are still not reported to getOwnPropertyDescriptors or getOwnPropertyNames. They are visible as keys in for .. in walk, but seemingly cannot be accessed with getOwnPropertyDescriptor either.

One possible solution is to directly set the IntersectionObserverEntry object as state, this would preserve the lazy loading of the expensive properties, but it would eliminate the current browser support fallback for isIntersecting.

Another solution would be to manually copy the known properties:

setIntersectingEntry({
  isIntersecting: entry.isIntersecting || entry.intersectionRatio > 0,
  intersectionRatio: entry.intersectionRatio,
  intersectionRect: entry.intersectionRect,
  /* etc */
});

However as it may be a performance degradation to automatically include evaluated values of these properties, I don't think it is wise to copy them all. Perhaps as a compromise solution we could leave out the DOMRectReadOnly properties (boundingClientRect, intersectingRect, and rootBounds), and manually copy the other simple values.

I can create a PR for either of these solutions, but am curious how others might prefer to proceed. Thanks!

Consuming repo

What repo were you working in when this issue occurred?

start-web

[ismail-syed]

However as it may be a performance degradation to automatically include evaluated values of these properties

I don't see how this would be much of a performance degradation. The original intention was to spread the object properties, but as you mentioned,

IntersectionObserverEntry are non-enumerable, and the current implementation tries to spread them across an internal state setter which fails

I think it should be safe to manually copy the properties over. Curious to hear what others think, cc/ @Shopify/web-foundation

[jamesism]

I don't see how this would be much of a performance degradation. The original intention was to spread the object properties, but as you mentioned,

Only worry is that this being scroll bound, some use cases that may not actually require the DOMRectReadOnly values might cause a slowdown on slower CPU devices when they are not explicitly used, especially with a low callback threshold. But I have no data to back this up :-1: .

[lemonmade]

Whether it's a performance issue or not it feels like we should stay away from copying the properties. I'd probably feel most comfortable if we saved the IntersectionObserverEntry, and just patch the instances to have the field if we see that the field is missing. Or, we could make this a polyfill instead, and patch IntersectionObserverEntry.prototype.

[jamesism]

Looks like the current polyfill brought in with @shopify/polyfills handles the missing isIntersecting. So setting the IntersectionObserverEntry directly is probably a safe bet :+1: