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 19 + Cloudflare Adapter -> "Bug: Uncaught ReferenceError: MessageChannel is not defined" #12824

Open
1 task
ethanniser opened this issue Dec 24, 2024 · 7 comments
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)

Comments

@ethanniser
Copy link

Astro Info

Astro                    v5.1.1
Node                     v22.6.0
System                   macOS (arm64)
Package Manager          bun
Output                   server
Adapter                  @astrojs/cloudflare
Integrations             @astrojs/mdx
                         @astrojs/sitemap
                         @astrojs/tailwind
                         @astrojs/react

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

originally shared facebook/react#31827 but I think it makes sense to be an issue here as I doubt its a react issue

image

It doesn't happen during build but instead during the "Deploying to Cloudflare's global network" step

What's the expected result?

The error should not happen

Link to Minimal Reproducible Example

https://github.com/phase/test-astro-react19-cf-workers (thank you @phase)

Participation

  • I am willing to submit a pull request for this issue.
@phase
Copy link

phase commented Dec 26, 2024

here's a workaround withastro/adapters#436 (comment)

astro.config.mjs

// @ts-check
import { defineConfig } from "astro/config";
import cloudflare from "@astrojs/cloudflare";

import react from "@astrojs/react";

// https://astro.build/config
export default defineConfig({
  output: "server",
  adapter: cloudflare(),
  integrations: [react()],
  vite: {
    resolve: {
      // Use react-dom/server.edge instead of react-dom/server.browser for React 19.
      // Without this, MessageChannel from node:worker_threads needs to be polyfilled.
      alias: import.meta.env.PROD && {
        "react-dom/server": "react-dom/server.edge",
      },
    },
  },
});

@acnebs
Copy link

acnebs commented Jan 5, 2025

As it seems unlikely this will be upstream fixed by Cloudflare this probably needs to be addressed on the Astro end.

@IgorMinar
Copy link

IgorMinar commented Jan 15, 2025

This turns out not to be a Cloudflare issue at all, but rather an odd misconfiguration of of Vite in Astro's Cloudflare plugin.

The culprit appears to be these lines:

https://github.com/withastro/adapters/blob/7b352ee1e52eaa96e6f0dff8be6de573ed7c46c7/packages/cloudflare/src/index.ts#L221-L226

Note how the Astro's adapter here aliases react-dom/server to react-dom/server.browser, which then contains the offending MessageChannel API call which throws because it's not defined.

I'm not sure why Astro created this alias in the first place, but this appears to me to be either obsolete workaround for some no longer present issue, or incorrect workaround for some existing issue. This alias was introduced in the initial commit of the adapter and there are no comments that I can see describing why it exists at all:

Changing this alias to point to react-dom/server.edge instead of react-dom/server.browser is enough to build the application and deploy it to cloudflare: https://a811a373.astro-5-test.pages.dev/

I think it's not right that this alias exist in the first place though, so I dug a but more and tried to remove it which surfaced a new issue related to react-dom/server being externalized and not bundled into the worker bundle which then causes a new deployment failure related to react-dom/server failing to resolve on Cloudflare.

Digging a bit more, I realized that even though the plugin currently tells vite to disable any externals:
https://github.com/withastro/adapters/blob/7b352ee1e52eaa96e6f0dff8be6de573ed7c46c7/packages/cloudflare/src/index.ts#L244

It then also proceeds to configure externals with a code that seems to be mistakingly copy pasted from the Astro Vue adapter:

https://github.com/withastro/adapters/blob/7b352ee1e52eaa96e6f0dff8be6de573ed7c46c7/packages/cloudflare/src/index.ts#L249-L256

This then results in react-dom/server to appearing in externals: [...] array in the final Vite config:

  vite:config     ssr: {
  vite:config       noExternal: true,
  vite:config       external: [ 'react-dom/server', 'react-dom/client' ],
  vite:config       resolve: {
  vite:config         conditions: [
  vite:config           'module',
  vite:config           'browser',
  vite:config           'development|production',
  vite:config           'workerd',
  vite:config           'worker',
  vite:config           'astro',
  vite:config           'astro'
  vite:config         ]
  vite:config       },
  vite:config       target: 'webworker'
  vite:config     },

This configuration is invalid, as workerd/Cloudflare doesn't allow for any npm packages to be externalized during bundling. All deps must be inlined, and thus the externals should be set to [] explicitly since that setting takes precedence over noExternals.


EDIT2: in the original comment I forgot to mention explicitly that to get things working I also had to set externals to [] so that all dependencies are bundled. I updated the paragraph above after a comment from @bluwy below.


So the right thing to do here, is to remove the externals config copy-pasted from vue mentioned above, and allow for react-dom/server to be bundled along with the application.

This then results again a correct bundle and working astro app.

One last thing to mention, is that since to configure conditions we append workerd and worker to the conditions array via push:
https://github.com/withastro/adapters/blob/7b352ee1e52eaa96e6f0dff8be6de573ed7c46c7/packages/cloudflare/src/index.ts#L241

This results in the vite config looking like this:

  vite:config     ssr: {
  vite:config       noExternal: true,
  vite:config       external: [ 'react-dom/server', 'react-dom/client' ],
  vite:config       resolve: {
  vite:config         conditions: [
  vite:config           'module',
  vite:config           'browser',
  vite:config           'development|production',
  vite:config           'workerd',
  vite:config           'worker',
  vite:config           'astro',
  vite:config           'astro'
  vite:config         ]
  vite:config       },
  vite:config       target: 'webworker'
  vite:config     },

Note that the conditions lists browser before workerd. While it's not well documented in vite docs it is my understanding that the order in this array matters, and conditions listed earlier will have higher precedence than conditions listed later. Fortunately this doesn't seem to cause problems in this simple Astro repro application from @phase. But I think it would be good to change the code to use unshift instead of push to prepend workerd to the array, rather than append it.

EDIT: I don't believe this is a problem, the conditions resolution algorithm is documented at https://esbuild.github.io/api/#how-conditions-work and states that it's only the order in package.json that matters. Thanks to @vicb and @dario-piotrowicz for pointing this out.


So to sum up my resolution recommendation:

  • remove the alias config
  • remove the externals config and set externals to []

Could someone from the Astro team look over these findings, and confirm this sounds right?

I have no idea why the alias was added to the plugin in the first place, but I can see that up until React 19 this didn't cause any problems so the alias was just sitting there not causing significant issues, and it was only with React 19 introducing MessageChannel code in the react-dom/server.browser module that this alias resulted in a worker bundle that blows up in the workerd runtime.

@ematipico
Copy link
Member

Can't git blame help explaining the reason why certain things were added? I believe the PRs associated with them can provide some useful context

@IgorMinar
Copy link

Nope. See my links above to commits introducing this alias.

@bluwy
Copy link
Member

bluwy commented Jan 16, 2025

Thanks for investigating this and pointing us to the right direction @IgorMinar. The proposal makes sense to me. I think we should try to remove the alias and allow react to resolve itself, though there were reports of that causing a different issue (withastro/adapters#436 (comment)) but maybe needs revisiting again.

About the externals option, I don't know if removing that code would fix the issue. It's only filtering based on existing configuration, so the externals are already there. The vue code is from withastro/adapters#210. If we want to not allow externalizing anything at all, we probably have to mutate the final vite config and make it an empty array completely.

Though, another approach to fixing the externals issue is to remove these hardcoded externals. I don't think it needs to be set by default and a similar fix was also done before (#10601).

@bluwy bluwy added - P4: important Violate documented behavior or significantly impacts performance (priority) and removed needs triage Issue needs to be triaged labels Jan 16, 2025
@IgorMinar
Copy link

You are right @bluwy! In my local changes I did set the externals to [] but forgot to document it! I'll update my comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)
Projects
None yet
Development

No branches or pull requests

6 participants