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

Add commonjs build #49

Closed
wants to merge 5 commits into from
Closed

Add commonjs build #49

wants to merge 5 commits into from

Conversation

cea2aj
Copy link
Member

@cea2aj cea2aj commented Oct 28, 2021

Add a commonjs build so that the library is compatible with node

There is a warning about useLayoutEffect since that is not compatible with server-side-rendering, however it still works. It will lead to a mismatch between the initial non-hydrated UI and the intended UI. If we do want to fully support that use case, there is more info here: https://gist.github.com/gaearon/e7d97cdf38a2907924ea12e4ebdf3c85, however that is out of scope of this item

J=SLAP-1666
TEST=manual

Use create-next-app and add answers-headless-react to it and test that a simple universal search works.

@coveralls
Copy link

coveralls commented Oct 28, 2021

Coverage Status

Coverage remained the same at 75.269% when pulling 51305da on dev/add-commonjs into 37050f2 on main.

@tmeyer2115
Copy link
Contributor

Add a commonjs build so that the library is compatible with node

There is a warning about useLayoutEffect since that is not compatible with server-side-rendering, however it still works. It will lead to a mismatch between the initial non-hydrated UI and the intended UI. If we do want to fully support that use case, there is more info here: https://gist.github.com/gaearon/e7d97cdf38a2907924ea12e4ebdf3c85, however that is out of scope of this item

J=SLAP-1666 TEST=manual

Use create-next-app and add answers-headless-react to it and test that a simple universal search works.

Can you elaborate a bit more about this mismatch? Is this something all uses of the node version would encounter? Would it be noticeable to them?

@cea2aj
Copy link
Member Author

cea2aj commented Oct 29, 2021

Can you elaborate a bit more about this mismatch? Is this something all uses of the node version would encounter? Would it be noticeable to them?

Server-side rendering works by creating an initial rendered HTML page and sending it to the client. The client will render that static page, and then the UI will become interactive once React starts running on the page (hydration). The server can't run run useEffect or useLayoutEffect, so any HTML it sends for the initial load won't have any effects applied to them. Since useAnswersState uses useLayoutEffect, the server can't include that data in the initial load that it sends to the client.

We could use https://www.npmjs.com/package/react-layout-effect so that useEffect is used instead of useLayoutEffect on the server. I'm not quite sure why they log an error for useLayoutEffect but not for useEffect, but I think it has to do with the guarantees that useLayoutEffect makes that useEffect doesn't, and the fact that useEffect is preferred when possible. Still, that would just fix the warning, but the server still wouldn't be able to render the page content since we'd still be using useEffect as part of the useAnswersState hook.

Taking a step back, how useful would SSR be for an answers experience? The server can render pages at build time like a SSG, however that doesn't make much sense for answers since the content is dynamic and based on the user's query. I could see it being useful on experiences with a default initial search though. Servers can also render initial pages per request. For example, if someone loads a page with certain query params, NextJS could be configured to fetch the data necessary to render that page so that data will be included in the initial HTML it sends to the client. We'd need to talk to product to see if this is a use case that they'd want the library to handle out of the box. Getting full support for that would require us write our components in such a way that they can fetch data and render it without using useEffect or useLayoutEffect, which would be a big change.

The most likely use case that I imagine for using headless with SSR is when it's being plugged into a site which is already heavily leveraging SSR. In that case we'd want to update the lib to avoid the warning, however the answers portion of the experience wouldn't show any data until React starts on the client.

@oshi97
Copy link
Contributor

oshi97 commented Oct 29, 2021

This is what react-redux does for react on the server side, not saying we should do this just adding more context
(it's just using useEffect in node environments like Connor also mentioned, the source code is almost identical to react-layout-effect)
https://github.com/reduxjs/react-redux/blob/master/src/utils/useIsomorphicLayoutEffect.ts

@tmeyer2115
Copy link
Contributor

This is what react-redux does for react on the server side, not saying we should do this just adding more context (it's just using useEffect in node environments like Connor also mentioned, the source code is almost identical to react-layout-effect) https://github.com/reduxjs/react-redux/blob/master/src/utils/useIsomorphicLayoutEffect.ts

@cea2aj, would the isomorphic useLayoutEffect or useIsomorphicLayoutEffect allow the static HTML to be generated properly and then the dynamic useAnswersState to function once the page is hydrated? I will check with Product about how much time we should be spending on SSR of React Components. I definitely see the value in having answers-headless work with Node, but am not fully sold on SSR of React.

@cea2aj
Copy link
Member Author

cea2aj commented Nov 1, 2021

@cea2aj, would the isomorphic useLayoutEffect or useIsomorphicLayoutEffect allow the static HTML to be generated properly and then the dynamic useAnswersState to function once the page is hydrated? I will check with Product about how much time we should be spending on SSR of React Components. I definitely see the value in having answers-headless work with Node, but am not fully sold on SSR of React.

No, those functions would just fix the warning in the console, but they wouldn't improve the capabilities of the initial render. We'd have to test out the components with SSR to see if the components look broken. At worst, we'll have to disable SSR of the components and let client side React render the components

@cea2aj
Copy link
Member Author

cea2aj commented Nov 2, 2021

We will hold off on merging this until product decides they want this functionality (Therefore marking as WIP)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants