alexkatz / react-tiny-popover

A simple and highly customizable popover react higher order component with no other dependencies! Typescript friendly.
MIT License
454 stars 121 forks source link

support server side rendering #108

Closed meikidd closed 3 years ago

meikidd commented 3 years ago

Thank you @alexkatz for making this awesome package.

I am currently migrating my frontend project to server side rendering project. These few lines change can make this package work on server side.

It would be super cool if you can review and merge this PR.

chrisvariety commented 3 years ago

would love to see this merged, seeing the same issue.

alexkatz commented 3 years ago

Closing this, as the PR violates a basic principle of hooks.

chrisvariety commented 3 years ago

@alexkatz which principle?

alexkatz commented 3 years ago

Hooks cannot be called conditionally. React relies on the same number of hooks within a component being called in the same order every render, in order to keep track of them.

An early return statement based on a particular condition means that every hook to follow is called conditionally. This will break your app.

alexkatz commented 3 years ago

In regard to server side rendering, I recommend creating an HOC that handles it. It can look something like this:

const SafePopover: FC<PopoverProps> = props => {
  if (typeof window === 'undefined') return props.children;
  return <Popover {...props} />;
};
chrisvariety commented 3 years ago

SSR isn't conditional though, does not violate the principle you have cited.

GusRuss89 commented 3 years ago

@alexkatz why not export SafePopover as the default export from this library? As of now, anyone using this library in Next.js will google the error and end up at #118 which doesn't currently include an answer for this question. Next.js is popular, you're probably losing a lot of users.