Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added guard against HTMLElement in callSubscriber #62

Closed
wants to merge 1 commit into from

Conversation

netsgnut
Copy link

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
Copy link
Owner

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

@netsgnut
Copy link
Author

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

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
Copy link

@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
Copy link
Owner

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. 🙏

@oleg-slapdash
Copy link

@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
Copy link

urish commented Jul 7, 2021

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

ZeeCoder added a commit that referenced this pull request Jul 27, 2021
…cases.

Passing in a custom ref would make the hook check for `instanceof HTMLElement`,
which fails in SSR environments. (Node, JSDom)

Fixes #74 #62
ZeeCoder added a commit that referenced this pull request Jul 27, 2021
…cases.

Passing in a custom ref would make the hook check for `instanceof HTMLElement`,
which fails in SSR environments. (Node, JSDom)

Fixes #74 #62
github-actions bot pushed a commit that referenced this pull request Jul 27, 2021
## [7.0.1](v7.0.0...v7.0.1) (2021-07-27)

### Bug Fixes

* Removed unnecessary entries.length check ([3211d33](3211d33))
* Undefined HTMLElement is no longer an issue in certain SSR edge cases. ([599cace](599cace)), closes [#74](#74) [#62](#62)
@ZeeCoder
Copy link
Owner

This is now fixed in 7.0.1! 🎉

The related commit in case anyone is interested in the final solution: 599cace

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

@ZeeCoder ZeeCoder closed this Jul 27, 2021
@urish
Copy link

urish commented Jul 27, 2021

Thank you!

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.

5 participants