use-resize-observer icon indicating copy to clipboard operation
use-resize-observer copied to clipboard

Added host to hooks for instanceof check

Open nerocui opened this issue 2 years ago • 9 comments

Hi @ZeeCoder , thanks for creating this library. We are using this package, and we'd like to help make it better. Our use case includes passing in child window's window object. instanceof checks the prototype, and that's unique on each window. So to make sure instanceof works reliably, we need to pass in the host object and compare to the correct Element. Default case is the global window object.

Hope you could give this PR a look when you are free. Thank you!

nerocui avatar Dec 16 '22 04:12 nerocui

Hi @nerocui :wave:

First of all: thank you, I really appreciate the effort here!

Can you provide a reproduction of your issue in codesandbox, so that I better understand your use-case please? :pray:

ZeeCoder avatar Dec 16 '22 09:12 ZeeCoder

@ZeeCoder Happy new year. Hope you had a good holiday break. I was not able to get it to work with codesandbox, but I uploaded a minimal repro to here https://github.com/nerocui/host-repro It's just a express app that host two static html pages. Essentially it's just showing that instanceof returns false if an element is created from a different window. So in order to get instanceof to work reliably, it needs to be called with the type from the host window.

Please let me know if this helps.

nerocui avatar Jan 03 '23 22:01 nerocui

Sorry I didn't come back to you yet, I'm extremely busy with personal stuff right now but I just wanted you to know I appreciate the time you've invested here. Seems like the issue stems from useResolvedElement's use of instanceof here: https://github.com/ZeeCoder/use-resize-observer/blob/490429fd909fe7ff6e65fbf30874b43cd76c24bc/src/utils/useResolvedElement.ts#L30-L37

I think one solution would be to change this:

    const element: T | null = cbElement
      ? cbElement
      : refOrElement
      ? refOrElement instanceof Element
        ? refOrElement
        : refOrElement.current
      : null;

To this:

    const element: T | null = cbElement
      ? cbElement
      : refOrElement
      ? "current" in refOrElement
        ? refOrElement.current
        : refOrElement
      : null;

:point_up: Basically by not using instanceof at all, we circumvent the issue altogether.

Could be a minor release. :thinking:

ZeeCoder avatar Feb 18 '23 19:02 ZeeCoder

Would need an additional test case ofc to ensure there'd be no regressions. Which is where most of the work lies tbh.

ZeeCoder avatar Feb 18 '23 19:02 ZeeCoder

@ZeeCoder Thank you so much for looking into it! Yes getting rid of interaction with global objects would solve this issue.

nerocui avatar Feb 18 '23 22:02 nerocui

I'll update the PR with the recommendation and test cases.

nerocui avatar Feb 18 '23 22:02 nerocui

Alternatively, using hasOwnProperty:

    const element =
      cbElement ||
      (refOrElement
        ? Object.prototype.hasOwnProperty.call(refOrElement, 'current')
          ? refOrElement.current
          : refOrElement
        : null)

bpinto avatar Feb 22 '23 15:02 bpinto

@bpinto looks about right, it's a shame that TS doesn't handle it properly though: image I mean there must be a reason why this isn't working. :thinking:

ZeeCoder avatar Feb 24 '23 19:02 ZeeCoder

Sorry @ZeeCoder, I have 0 experience with TS but a google search showed me the following solution:

Type assertion since we know it's a ref and not an element/state:

const element =
  cbElement ||
  (refOrElement
    ? Object.prototype.hasOwnProperty.call(refOrElement, 'current')
      ? (refOrElement as RefObject<T>).current
      : refOrElement as T
    : null)

I tested it here

bpinto avatar Feb 26 '23 16:02 bpinto