This repository has been archived by the owner on Dec 30, 2022. It is now read-only.
instantsearch-hooks-server: use literal imports #3618
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b181c45:
|
✅ Deploy Preview for react-instantsearch ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Next.js build process (using pnpm) ends up not seeing these import statements cause the import statements didn't have string literals. The result was that it didn't put these dependencies in the nested `node_modules` directory and thus this whole package breaks. Notably this only fails in the production build and not the dev build. This change is equivalent, we're just reformatting the code so the literal import statements can be found by whatever tool Next.js is using.
danielbeardsley
force-pushed
the
react-imports--use-literal-imports
branch
from
54dc34c
to
b181c45
Compare
|
Argh, it's annoying that this code breaks again, as you can see in the linked issue webpack/webpack#13865. We'll need to investigate this deeply to find the right solution, or possibly make the import user-injected (dependency injection) so it can't have any of these issues. More specifically it's changed from import statements to expressions in #3515 |
|
@danielbeardsley, for a workaround you can use patch-package for now I think, is there a way we can reproduce this outside of Vercel? |
|
Maybe my pullrequest will work: |
Yeah, we started using |
Haroenv
added a commit
that referenced
this pull request
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/instantsearch
that referenced
this pull request
Jan 4, 2023
…eact-instantsearch#3658) **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 algolia/react-instantsearch#3633 closes algolia/react-instantsearch#3618 see vercel/next.jsalgolia/react-instantsearch#40067 FX-1869 Co-authored-by: François Chalifour <francoischalifour@users.noreply.github.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Next.js build process (using pnpm) ends up not seeing these import statements cause the import statements didn't have string literals.
The result was that it didn't put these dependencies in the nested
node_modulesdirectory and thus this whole package breaks. Notably this only fails in the production build and not the dev build. See reported bug at next.js.This change is functionally equivalent, we're just reformatting the code so the literal import statements can be found by whatever tool Next.js is using.