withastro / adapters

Home for Astro's core maintained adapters
72 stars 44 forks source link

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

Closed dallyh closed 6 months ago

dallyh commented 8 months ago

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

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

alexanderniebuhr commented 8 months ago

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 commented 8 months ago

Just checking in :)

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

deeprobin commented 7 months ago

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 commented 7 months ago

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

dallyh commented 7 months ago

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

alexanderniebuhr commented 7 months ago

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

dallyh commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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

Please try @astrojs/cloudflare@0.0.0-cf-deps-chunk-20240406183753

dallyh commented 7 months ago

@alexanderniebuhr Same as in https://github.com/withastro/adapters/issues/213#issuecomment-2041220402 :(

dallyh commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

It could potentially be differences between operating systems, or may be that the @astrojs/cloudflare@0.0.0-cf-deps-chunk-20240406183753 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/astro@4.5.16_@types+node@20.12.5_typescript@5.4.4/node_modules/astro/dist/core/build/generate.js:96:16)
    at async AstroBuilder.build (file:///C:/Users/honda/git/daliborhon.dev/node_modules/.pnpm/astro@4.5.16_@types+node@20.12.5_typescript@5.4.4/node_modules/astro/dist/core/build/index.js:134:5)
    at async build (file:///C:/Users/honda/git/daliborhon.dev/node_modules/.pnpm/astro@4.5.16_@types+node@20.12.5_typescript@5.4.4/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 commented 7 months ago

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 commented 7 months ago

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

@astrojs/cloudflare@0.0.0-cf-deps-chunk-20240407075425

dallyh commented 7 months ago

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 https://github.com/withastro/adapters/issues/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 https://github.com/withastro/adapters/issues/220 arises as all the chunks are still included, but just not used server side.

alexanderniebuhr commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago
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/cloudflare@0.0.0-cf-deps-chunk-20240407075425 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 commented 6 months ago

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