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

Algolia instantsearch react-dom dependency pruned incorrectly during build phase #40067

Open
1 task done
federicobadini opened this issue Aug 29, 2022 · 1 comment
Open
1 task done
Labels
bug Issue was opened via the bug report template.

Comments

@federicobadini
Copy link

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
Platform: darwin
Arch: arm64
Version: Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:37 PDT 2022; root:xnu-8020.121.3~4/RELEASE_ARM64_T6000
Binaries:
Node: 16.15.1
npm: 8.11.0
Yarn: 1.22.15
pnpm: 6.34.0
Relevant packages:
next: 12.2.6-canary.6
eslint-config-next: 12.0.7
react: 18.2.0
react-dom: 18.2.0

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

Vercel

Describe the Bug

Releases >= v6.28.0 of react-instantsearch-hooks have modified the way in which they import react-dom (old implementation - new implementation)

The new import syntax is causing issues on Vercel if the project dependencies are handled via pnpm.

It seems that during the vercel build phase, an over-aggressive tree-shaking causes the loss of the dependency between react-instantsearch-hooks-server and react-dom.

On locally installed node_modules the dependency is present:

symlink-to-react-dom-in-local-dependencies

On the contrary, if we inspect the node_modules generated by npx vercel build the react-dom symlink is not preserved:

no-symlink-to-react-dom

This is causing errors in Vercel due to the fact that react-dom cannot be found.

Expected Behavior

We expect that the build command is not removing dependencies that are needed for the project to run

Link to reproduction

https://github.com/federicobadini/instantsearch-vercel-test

To Reproduce

Neither StackBlitz nor Codesandbox allow to handle dependencies with pnpm without errors.
As a consequence I can only provide a link to the public repo we used for debugging this issue - which is basically a clone of the react-instantsearch-hooks sample.

If executed locally the project builds and runs correctly.

If deployed on Vercel it produces a 500 error: Error: Could not import ReactDOMServer

@federicobadini federicobadini added the bug Issue was opened via the bug report template. label Aug 29, 2022
@federicobadini federicobadini changed the title Algolia instantsearch dependency pruned incorrectly during build phase Algolia instantsearch react-dom dependency pruned incorrectly during build phase Aug 29, 2022
@danielbeardsley
Copy link

Opened a pull on react-instantsearch I'll be testing it out: algolia/react-instantsearch#3618

Haroenv added a commit to algolia/react-instantsearch that referenced this issue Oct 19, 2022
There are some cases where the combination of trying to make sure renderToString doesn't end up in a browser bundle, being runnable on esm + cjs, react 17 and 18, .js extension etc. blows up. One of those is pnpm caching removing "unused" packages.

This PR introduces a new argument `renderToString` to `getServerState` so you can inject the dependency yourself, meaning the import is within your own code and won't be purged.

```js
import { renderToString } from 'react-dom/server';
await getServerState(<App/>, renderToString);
await getServerState(<App/>, import('react-dom/server').then(mod => mod.renderToString));
```

fixes #3633
closes #3618
see vercel/next.js#40067
Haroenv added a commit to algolia/react-instantsearch that referenced this issue Oct 20, 2022
**Summary**

There are some cases where the combination of trying to make sure
renderToString doesn't end up in a browser bundle, being runnable on esm
+ cjs, react 17 and 18, .js extension etc. blows up.

One of those is pnpm/vercel removing "unused" packages.




<!--
  Thanks for submitting a pull request!
Please provide enough information so that others can review your pull
request.
-->


<!--
  Explain the **motivation** for making this change.
  What existing problem does the pull request solve?
  Are there any linked issues?
-->

**Result**


This PR introduces a new argument `renderToString` to `getServerState`
so you can inject the dependency yourself, meaning the import is within
your own code and won't be purged.

```js
import { renderToString } from 'react-dom/server';
await getServerState(<App/>, renderToString);
await getServerState(<App/>, import('react-dom/server').then(mod => mod.renderToString));
```

<!--
  Demonstrate the code is solid.
  Example: The exact commands you ran and their output,
  screenshots / videos if the pull request changes UI.
-->

fixes #3633
closes #3618
see vercel/next.js#40067
FX-1869

Co-authored-by: François Chalifour <francoischalifour@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Development

No branches or pull requests

2 participants