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

Cloudflare v10 breaks cloudflare build / wrangler preview with React #211

Closed
1 task
dallyh opened this issue Mar 29, 2024 · 23 comments · Fixed by #226
Closed
1 task

Cloudflare v10 breaks cloudflare build / wrangler preview with React #211

dallyh opened this issue Mar 29, 2024 · 23 comments · Fixed by #226
Labels
- P4: important Violate documented behavior or significantly improves performance (priority) pkg: cloudflare upstream

Comments

@dallyh
Copy link

dallyh commented Mar 29, 2024

Astro Info

Astro                    v4.5.12
Node                     v20.12.0
System                   Windows (x64)
Package Manager          npm
Output                   hybrid
Adapter                  @astrojs/cloudflare
Integrations             @astrojs/react

Describe the Bug

When using v10 version of the adapter, hybrid output mode and React on a prerendered path with client:load directive, preview with wrangler fails. This was not the case with previous version of the adapter. I haven't tried building the site on Cloudflare, but my guess is if the wrangler fails, then the build will fail too.

Steps to reproduce

  • npm install
  • npm build
  • npm preview
Attaching additional modules:
┌──────────────────────────────────────────┬──────┬───────────┐
│ Name                                     │ Type │ Size      │
├──────────────────────────────────────────┼──────┼───────────┤
│ chunks/astro/assets-service_DrRUv88a.mjs │ esm  │ 10.73 KiB │
├──────────────────────────────────────────┼──────┼───────────┤
│ chunks/astro_lYPtO5er.mjs                │ esm  │ 83.18 KiB │
├──────────────────────────────────────────┼──────┼───────────┤
│ chunks/generic_CIq48spH.mjs              │ esm  │ 0.19 KiB  │
├──────────────────────────────────────────┼──────┼───────────┤
│ chunks/index_B6euTALA.mjs                │ esm  │ 0.18 KiB  │
├──────────────────────────────────────────┼──────┼───────────┤
│ chunks/pages/generic_DCwcQpq8.mjs        │ esm  │ 43.64 KiB │
├──────────────────────────────────────────┼──────┼───────────┤
│ chunks/pages/index_CPLiUgeG.mjs          │ esm  │ 0.20 KiB  │
├──────────────────────────────────────────┼──────┼───────────┤
│ chunks/prerender_Bqre5yrK.mjs            │ esm  │ 0.09 KiB  │
├──────────────────────────────────────────┼──────┼───────────┤
│ chunks/test_C85FZqRY.mjs                 │ esm  │ 0.19 KiB  │
├──────────────────────────────────────────┼──────┼───────────┤
│ chunks/vnode-children_DMYleglm.mjs       │ esm  │ 2.95 KiB  │
├──────────────────────────────────────────┼──────┼───────────┤
│ manifest_BqJYjMlt.mjs                    │ esm  │ 18.09 KiB │
├──────────────────────────────────────────┼──────┼───────────┤
│ renderers.mjs                            │ esm  │ 78.01 KiB │
├──────────────────────────────────────────┼──────┼───────────┤
│ _noop-middleware.mjs                     │ esm  │ 0.12 KiB  │
└──────────────────────────────────────────┴──────┴───────────┘
✨ Compiled Worker successfully
 ⛅️ wrangler 3.39.0
-------------------
[wrangler:inf] Ready on http://127.0.0.1:4321
⎔ Starting local server...
X [ERROR] service core:user:issues: Uncaught TypeError: Cannot read properties of undefined (reading 'ReactCurrentDispatcher')

    at null.<anonymous> (vowq86nvp6.js:3366:68) in .wrangler/tmp/pages-Phcscz/renderers.mjs
    at null.<anonymous> (vowq86nvp6.js:4:56) in __init
    at null.<anonymous> (vowq86nvp6.js:7422:1)


X [ERROR] MiniflareCoreError [ERR_RUNTIME_FAILURE]: The Workers runtime failed to start. There is likely additional logging output above.    

What's the expected result?

React should be usable on a prerendered path as it was like this before. If this is not the case, it should be mentioned in the docs that React should be used only with SSR enabled endpoint/path.

Link to Minimal Reproducible Example

https://github.com/dallyh/astro-cf-10-issues/tree/react-issue

Participation

  • I am willing to submit a pull request for this issue.
@dallyh dallyh changed the title Cloudflare v10 breaks build with React Cloudflare v10 breaks cloudflare build / wrangler preview with React Mar 29, 2024
@alexanderniebuhr alexanderniebuhr added pkg: cloudflare - P4: important Violate documented behavior or significantly improves performance (priority) labels Mar 29, 2024
@alexanderniebuhr
Copy link
Member

Thanks for reporting the issue. It should work, this is a regression. I was able to reproduce it and am working on a fix :)

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Apr 1, 2024

Just checking in :)

I was able to reproduce it, but still haven't found a fix, since it only happens for prerendered pages 🤔

@deeprobin
Copy link

As already mentioned in the Discord, this also applies to Solid & possibly other frameworks.

@alexanderniebuhr Have you been able to find out more or get to the bottom of the problem?

@alexanderniebuhr
Copy link
Member

@deeprobin Can you link the related Discord message which shows that this also applies to Solid

@alexanderniebuhr alexanderniebuhr added - P5: urgent Fix build-breaking bugs affecting most users, should be released ASAP (priority) and removed - P4: important Violate documented behavior or significantly improves performance (priority) labels Apr 5, 2024
@dallyh
Copy link
Author

dallyh commented Apr 5, 2024

@alexanderniebuhr It will be this one https://discord.com/channels/595317990191398933/1223238165918384269, I was following this thread too.

@alexanderniebuhr
Copy link
Member

So I found a workaround, I know it's not create but if you have at least one page which is SSR using a React component with client:load, everything should work. So until we found the real fix, maybe you can add a SSR page with using a simple react component with client:load

@alexanderniebuhr alexanderniebuhr added - P4: important Violate documented behavior or significantly improves performance (priority) and removed - P5: urgent Fix build-breaking bugs affecting most users, should be released ASAP (priority) labels Apr 5, 2024
@dallyh
Copy link
Author

dallyh commented Apr 6, 2024

For me the issues with version 10 are currently too much hassle, so I'm personally staying on v9. Here is documentation for the older version, for anyone else still on that version.

I believe that v10 will be great eventually, it really fits with the recent Cloudflare changes like that wrangler.toml can now be used for configuring a Pages project, however it uncovered some upstream issues, some of which are a little bit bigger and will probably take some time to properly fix. Still, thanks for your hard work guys! Really appreciate it.

@alexanderniebuhr
Copy link
Member

That's totally fair and also the reason why we are still maintaining v9 with security fixes and urgent bug fixes for some weeks. We hope that all the upstream issues are fixed by the time v9 maintenance ends.

@dallyh
Copy link
Author

dallyh commented Apr 6, 2024

I was looking at the built chunks, and i may have found something. When the page is built without prerender = true and client:.. on page which contains React, the outputted renderers.mjs imports r as reactExports from ./chunks/prerender_xxxxx.mjs. This file contains only this:

const noop = () => {};
export const R = noop;
export const r = noop;
export const t = noop;

Thus next this variable var aa$1=reactExports contains nothing, so of course the line where it fails aa$1.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactCurrentDispatcher is undefined.

When the page is built with prerender = false and client:.. directive, the outputted renderers.mjs imports r as reactExports from ./chunks/pages/test_xxxxx.mjs'. This file then contains export { React as R, reactExports as r, test as t }; and it looks like it correctly exports all of the required React libraries and modules.

The minification makes it hard, but I have found that the object is getting set in the code

var react_production_min = {};

...

W={ReactCurrentDispatcher:U,ReactCurrentBatchConfig:V,ReactCurrentOwner:K}

...

react_production_min.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED=W;
...

{
  react.exports = react_production_min;
}

var reactExports = react.exports;

...

export { React as R, reactExports as r, test as t };

So basically where there is no on demand page with react and client directive, the imports leads to nowhere?

@alexanderniebuhr
Copy link
Member

@dallyh That is actually very helpful, thank you. We now have something to look into more detail. renderers.mjs shouldn't be needed for prerendered pages, because everything should be already in the .html file, but maybe the issue is that we still import it somehow.

@dallyh
Copy link
Author

dallyh commented Apr 6, 2024

Adding this to the Astro config adds all of the React stuff into single outputted chunk file, then everything looks to work fine. So I'm just dropping this here as a better workaround than setting up an ondemand page with React and client directive :)

    vite: {
        build: {
            rollupOptions: {
                output: {
                    manualChunks: (id) => {
                        //console.log(id);
                        if (id.includes("node_modules/react")) {
                            return "react-chunk";
                        }
                    },
                },
            },
        },
    },

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Apr 6, 2024

@dallyh That was the missing piece! Thanks so much! ❤️

Please try @astrojs/[email protected]

@dallyh
Copy link
Author

dallyh commented Apr 6, 2024

@alexanderniebuhr Same as in #213 (comment) :(

@dallyh
Copy link
Author

dallyh commented Apr 6, 2024

I've dug deeper and the issue with the build seems to lie here https://github.com/withastro/astro/blob/ecb44351e98cfba575447699689b0ace49cd3c9d/packages/astro/src/runtime/server/escape.ts and here with this file https://github.com/WebReflection/html-escaper/blob/master/cjs/index.js in dependency html-escaper.

Changing the manual chunks to this fixes the build, and React seems to work fine.

manualChunks: (id) => {
                        if (id.includes("node_modules") && !id.includes("node_modules/astro") && !id.includes("node_modules/html-escaper")) return "vendor";
                    },

@alexanderniebuhr
Copy link
Member

Ok this is very strange. I have tested the build locally with your reproduction example and it worked without issues, I'll update the branch, but I'm afraid that depending on what users use in there project, that there might be more cases like this in the future.

Anyways it's super hard to reproduce for me.

@dallyh
Copy link
Author

dallyh commented Apr 7, 2024

It could potentially be differences between operating systems, or may be that the @astrojs/[email protected] version was built before this commit? Seems unlikely tho 😅, but the more I'm trying to help, the more I'm getting lost on this one.

edit:

As you said that there will be more cases, i¨ve tried the recent version in my project and got:

Cannot access 'createClient' before initialization
  Stack trace:
    at file:///C:/Users/honda/git/daliborhon.dev/packages/frontend/dist/_worker.js/chunks/prerender_qihfARuo.mjs:137:22
    at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)
    at async generatePages (file:///C:/Users/honda/git/daliborhon.dev/node_modules/.pnpm/[email protected]_@[email protected][email protected]/node_modules/astro/dist/core/build/generate.js:96:16)
    at async AstroBuilder.build (file:///C:/Users/honda/git/daliborhon.dev/node_modules/.pnpm/[email protected]_@[email protected][email protected]/node_modules/astro/dist/core/build/index.js:134:5)
    at async build (file:///C:/Users/honda/git/daliborhon.dev/node_modules/.pnpm/[email protected]_@[email protected][email protected]/node_modules/astro/dist/core/build/index.js:46:3)

With integration sanity-astro :( Maybe a better way of handling this would be to pick some of the packages like React, and create chunks for them, instead of picking everything in the node_modules directory?

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Apr 7, 2024

Can you show me the line of file:///C:/Users/honda/git/daliborhon.dev/packages/frontend/dist/_worker.js/chunks/prerender_qihfARuo.mjs:137:22?

Maybe a better way of handling this would be to pick some of the packages like React, and create chunks for them, instead of picking everything in the node_modules directory?

Then it's just the other way around, we would start with React, Solid, e.g.
But then other packages might fail which we have missed, and then we need to add them later...
So either it is add later or exclude later..

I'm trying to think about a solution, and hopefully we can cut a release early next week, with a version which does work good enough for now.

@alexanderniebuhr
Copy link
Member

@dallyh sorry for all this back and forth, but can you try this version with your real project?

@astrojs/[email protected]

@dallyh
Copy link
Author

dallyh commented Apr 7, 2024

You could ping me at discord, @dally.h for some testing, if required.

However, I tried it on the example React repo which works fine, then I tried it on #213 this issue/repo and my other projects, and it did not work because of the same errors with Shard and its dependencies. But again for whatever reason, adding "sharp" to manual chunks seems to do the trick.

adapter: cloudflare({
        imageService: "compile",
        platformProxy: {
            enabled: true,
        },
        experimental: {
            manualChunks: ["sharp"]
        }
    }),

With this tho the "sharp" chunk is created but not referenced anywhere, so then Wrangler just probably does not pick it up. So looks like we solved this one, but now #220 arises as all the chunks are still included, but just not used server side.

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Apr 7, 2024

Ok so maybe adding "sharp" by default makes sense. For #220, there is already a fix, but we need to wait for an upstream fix, otherwise we can't merge the adapter fix, but I don't know when that will land, or we need to find a workaround for the time being. Let me think about that 🤔

@hkbertoson
Copy link

CleanShot 2024-04-11 at 17 00 11

I am getting this error when trying with a React Component.

vite: {
    ssr: {
      external: ["node:buffer", "node:path", "node:fs", "node:os"],
    },
    build: {
      rollupOptions: {
        output: {
          manualChunks: (id) => {
            if (id.includes("node_modules/react")) {
              return "react-chunk";
            }
          },
        },
      },
    },
  },

@lukeojones
Copy link

lukeojones commented May 2, 2024

Astro                    v4.7.1
Node                     v22.1.0
System                     Mac M1 (Sonoma)
Package Manager          npm
Output                   server (with prerender on specific route)
Adapter                  @astrojs/cloudflare
Integrations             @astrojs/react

Fwiw, I was having a very similar issue when trying to pre-render pages and was able to workaround the problem by trying the @astrojs/[email protected] package.

I've been around the houses on this as, on the latest version of the Cloudflare adapter (@10.2.4), I was actually hit with a different issue which precluded me from seeing the ReactCurrentDispatcher is undefined problem.

In version 10.2.4, if I try and pre-render a page, the built output in renderers.mjs includes references to a non-existent chunks/prerender_[hash].mjs which appears to be moved/deleted during the build process.

Downgrading to 10.0.0 was what caused me to bump into this issue and, with the test package, I've been able to move forward.

@alexanderniebuhr
Copy link
Member

Yeah this import issue with prerendering has it's origins in Astro core 😢

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 improves performance (priority) pkg: cloudflare upstream
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants