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

[React 18] react-dom/server resolution on worker runtimes #2299

Closed
BasixKOR opened this issue Mar 12, 2022 · 5 comments
Closed

[React 18] react-dom/server resolution on worker runtimes #2299

BasixKOR opened this issue Mar 12, 2022 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@BasixKOR
Copy link
Contributor

BasixKOR commented Mar 12, 2022

What version of Remix are you using?

1.2.3

Steps to Reproduce

  • Install react-dom@rc react@rc on project using a worker runtime (such as Cloudflare Workers). I tested this on Cloudflare Pages.
  • Use renderToReadableStream on entry.server.js. Mine looks like this:
  // ... something ...
  const stream = await renderToReadableStream(
    <RemixServer context={remixContext} url={request.url} />
  );

  return new Response(stream, {
    status: responseStatusCode,
    headers: responseHeaders,
  });
  • Fails on Miniflare because Node.js globals aren't found.

Expected Behavior

It should load server.browser.js as it's specified on the worker exports field of react-dom.

Ok, I know this API isn't even released yet and is not meant to be used right now but I'm guessing Remix would likely adopt React 18 so I thought this is worth investigating.

This happens because esbuild has no target setting for Workers yet so Remix currently uses neutral which loads default export.

However, react-dom exports server.node.js by default which uses some Node.js-specific APIs such as Buffer, which isn't available on Cloudflare nor Miniflare. I'm not quite sure why this has worked before (maybe Node.js API calls were DCE'd because it isn't used for renderToString?), but when it is used with renderToReadableStream it doesn't work as intended.

This may need to be addressed on esbuild and thus may be considered as an upstream issue.

References

Actual Behavior

[pages:err] ReferenceError: Buffer is not defined
    at stringToPrecomputedChunk (C:\Users\basix\Projects\basixpage\node_modules\react-dom\cjs\react-dom-server.node.development.js:117:3)
    at C:\Users\basix\Projects\basixpage\node_modules\react-dom\cjs\react-dom-server.node.development.js:1674:25
    at node_modules/react-dom/cjs/react-dom-server.node.development.js (C:\Users\basix\AppData\Local\Temp\functionsWorker.js:16223:9)
    at __require (C:\Users\basix\AppData\Local\Temp\functionsWorker.js:42:50)
    at node_modules/react-dom/server.node.js (C:\Users\basix\Projects\basixpage\node_modules\react-dom\server.node.js:9:7)
    at __require (C:\Users\basix\AppData\Local\Temp\functionsWorker.js:42:50)
    at C:\Users\basix\Projects\basixpage\app\entry.server.tsx:1:40
    at SourceTextModule.evaluate (node:internal/vm/module:224:23)
    at VMScriptRunner.runAsModule (C:\Users\basix\Projects\basixpage\node_modules\@miniflare\runner-vm\src\index.ts:38:18)
    at VMScriptRunner.run (C:\Users\basix\Projects\basixpage\node_modules\@miniflare\runner-vm\src\index.ts:82:17)
Could not start Miniflare.

Your message may vary but it would likely fail on react-dom-server.node.something.js.

@BasixKOR BasixKOR added the bug Something isn't working label Mar 12, 2022
@BasixKOR
Copy link
Contributor Author

Or maybe we could alias react-dom/server to react-dom/server.browser on worker runtimes and call it a day.

@BasixKOR
Copy link
Contributor Author

BasixKOR commented Mar 13, 2022

It seems like esbuild allows to override exports condition fields to add worker condition to the compiler. Unsure if this should be treated as breaking change.

I'm willing to file a PR for this, so please let me know what do you think about this approach.

@BasixKOR
Copy link
Contributor Author

Should be fixed by #1977. Nice!

@styxlab
Copy link

styxlab commented Apr 7, 2022

Now that #1977 is fixed and published on 1.3.5-pre.3 I revisited this issue, but could not get it to work. With React 18.0.0-rc.3 and the following entry.server.tsx:

import { renderToReadableStream } from 'react-dom/server'
import { RemixServer } from 'remix'
import type { EntryContext } from 'remix'

export default async function handleRequest(
  request: Request,
  responseStatusCode: number,
  responseHeaders: Headers,
  remixContext: EntryContext
) {
  const stream = await renderToReadableStream(<RemixServer context={remixContext} url={request.url} />)

  return new Response(stream, {
    status: responseStatusCode,
    headers: responseHeaders,
  })
}

remix doesn't recognize the import { renderToReadableStream } from 'react-dom/server'. This looks similar to facebook/react#24226, but not sure how to deal with it in remix.

Interestingly, the above code works in dev mode, but not when deployed to cf-workers.

@mjackson
Copy link
Member

This should be fixed in the latest version of Remix which has options for configuring module resolution during the server build. Please update your remix.config.js to include this (or something similar, according to your needs):

{
  "serverConditions": ["workerd", "worker", "browser"]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Closed
Development

No branches or pull requests

4 participants