ZeeCoder / use-resize-observer

A React hook that allows you to use a ResizeObserver to measure an element's size.
MIT License
651 stars 42 forks source link

Added guard against HTMLElement in callSubscriber #62

Closed netsgnut closed 3 years ago

netsgnut commented 3 years ago

Hi!

Thank you for the library! We are using v7.0.0 in one of the Next.js projects recently.

In one of the few instances that we are using this library, we are providing a

export default function AComponent(): JSX.Element {
  const ref = useRef<HTMLDivElement>(null);
  useResizeObserver<HTMLDivElement>({
    ref,
    onResize: ({ width }) => {
      // Do something here
    },
  });
  return (
    <div ref={ref}>{/* snip */}</div>
  );
}

However when we try to build it with next build, there will be a ReferenceError: HTMLElement is not defined.

ReferenceError: HTMLElement is not defined
    at useResolvedElement (node_modules/use-resize-observer/dist/bundle.cjs.js:54:23)
    at useResizeObserver (node_modules/use-resize-observer/dist/bundle.cjs.js:113:21)

...which I believe is caused by the HTMLElement being referenced in the server-side, when it is trying to resolve the element.

Admittedly I am not sure if we are actually abusing/misusing the library, or if the patch is merely hiding some bugs/issues lying deeper within the logic, since in another component that uses the ref from the library (just like what README says):

export default function BComponent(): JSX.Element {
  const { ref } = useResizeObserver<HTMLDivElement>({
    onResize: ({ width }) => {
      // Do something here
    },
  });
  return (
    <div ref={ref}>{/* snip */}</div>
  );
}

...does not result in such error.

What do you think?

ZeeCoder commented 3 years ago

@netsgnut this syntax only works when you use TypeScript. Maybe that's the issue? :thinking:

netsgnut commented 3 years ago

Thank you for the swift response, @ZeeCoder ! While the code above is certainly TypeScript, I guess it is not really the reason here. :thinking:

I have set up a super minimal repo demonstrating this potential issue. I have used npx create-next-app without using the TypeScript preset, and on my machine (Linux x64, node v14.16.0), it would reproduce the ReferenceError: HTMLElement is not defined.

shobhitsharma commented 3 years ago

@ZeeCoder I can confirm this throws for us too. Unfortunately have to rewrite this only my own instead of including this package to solve it.

ZeeCoder commented 3 years ago

I get the issue now, seems like Next.js is using js-dom for SSR, which does not include the HTMLElement global. I do have a test for SSR, but that uses react-dom/server.

I'll need to refactor that test to replicate the issue before a fix is applied.

Thanks for flagging. :pray:

oleg-slapdash commented 3 years ago

@ZeeCoder @netsgnut Here is a repro, you need to use ForwardRef to get into this issue: node test.js test.js:

const React = require("react");
const ReactDOMServer = require("react-dom/server");
const useResizeObserver = require("use-resize-observer");

const ForwardRef = React.forwardRef(function (props, ref) {
  return React.createElement(
    "div",
    { style: { width: 100, height: 200 } },
    `${width}x${height}`
  );
});

function Test() {
  const ref = React.useRef(null);
  useResizeObserver({
    ref: ref,
  });
  return React.createElement(ForwardRef, { ref });
}

ReactDOMServer.renderToString(React.createElement(Test));
node -v
v15.1.0
% node test.js
/top-secret-project/node_modules/react-dom/cjs/react-dom-server.node.development.js:3392
            throw err;
            ^

ReferenceError: HTMLElement is not defined
    at useResolvedElement (/top-secret-project/node_modules/use-resize-observer/dist/bundle.cjs.js:54:49)
    at useResizeObserver (/top-secret-project/node_modules/use-resize-observer/dist/bundle.cjs.js:113:21)
    at Test (/top-secret-project/packages/client/src/test.js:16:3)
    at processChild (/top-secret-project/node_modules/react-dom/cjs/react-dom-server.node.development.js:3043:14)
    at resolve (/top-secret-project/node_modules/react-dom/cjs/react-dom-server.node.development.js:2960:5)
    at ReactDOMServerRenderer.render (/top-secret-project/node_modules/react-dom/cjs/react-dom-server.node.development.js:3435:22)
    at ReactDOMServerRenderer.read (/top-secret-project/node_modules/react-dom/cjs/react-dom-server.node.development.js:3373:29)
    at Object.renderToString (/top-secret-project/node_modules/react-dom/cjs/react-dom-server.node.development.js:3988:27)
    at Object.<anonymous> (/top-secret-project/packages/client/src/test.js:22:16)
    at Module._compile (node:internal/modules/cjs/loader:1083:30)
% node -v
urish commented 3 years ago

I can confirm the changes from this pull request fix the issue for me (also using Next.js SSR)

ZeeCoder commented 3 years ago

This is now fixed in 7.0.1! :tada:

The related commit in case anyone is interested in the final solution: https://github.com/ZeeCoder/use-resize-observer/commit/599cace5c33ecd4276a0fe2848e0ed920f81e2fe

Thanks for everyone involved in this issue, your contributions helped and somewhat guided the final solution. :pray:

urish commented 3 years ago

Thank you!