withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
45.59k stars 2.39k forks source link

🐛 BUG: react component + `client:load` fails with `preact/compat` but works fine with `@astrojs/react` #4107

Closed mayank99 closed 2 years ago

mayank99 commented 2 years ago

What version of astro are you using?

1.0.0-rc.3

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

npm

What operating system are you using?

Windows / Stackblitz

Describe the Bug

I have a ComboBox component from react-aria that I'm trying to render with client:load using the @astrojs/preact adapter with { compat: true }. It gives me the following error:

Unable to render ComboBox!

This component likely uses @astrojs/react, @astrojs/preact, @astrojs/vue
or @astrojs/svelte, but Astro encountered an error during server-side rendering.

Please ensure that ComboBox:
1. Does not unconditionally access browser-specific globals like `window` or `document`.
   If this is unavoidable, use the `client:only` hydration directive.
2. Does not conditionally return `null` or `undefined` when rendered on the server.

Using client:load='preact' works fine so I believe this is an SSR issue. However, I noticed that swapping out preact({ compat: true }) with just react() makes it work with client:load again.

(I did also come across https://github.com/withastro/astro/issues/3306, but this is react-aria not react-spectrum (which is a styled wrapper around react-aria) so there is no CSS involved. This could be why it works fine on server with react().)

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-xkptsr?file=src%2Fpages%2Findex.astro,astro.config.mjs

Participation

bluwy commented 2 years ago

I'll take this too! Blocked by #4093

bluwy commented 2 years ago

I believe #4213 should fix this too. I saw the same error while making the test and can confirm it's fixed with the PR.

mayank99 commented 2 years ago

it looks like this problem is still happening on astro@1.0.2 and @astrojs/preact@1.0.1 🙁 Getting the same error.

4093 also seems to be still broken.

bluwy commented 2 years ago

I was able to get dev working by specifying vite.ssr.noExternal: ['react-aria', 'react-stately', '@react-stately/*', '@react-aria/*']. Same for #4093 too with vite.ssr.noExternal: ['downshift']. The reason we need to do so is that in order for us to alias react > preact/compat, we need to process those libraries, instead of letting nodejs handle it.

I'd agree that it's a bit cumbersome, but I actually found a nicer workflow using this suggestion.

I'll re-open this PR, so we can suggest using npm:@preact/compat in the docs. That should make things configless, but for some reason it's failing the build, which I have to investigate.

bluwy commented 2 years ago

Well turns out the build fails because now preact suffers from dual package hazard. Astro calls preact-render-to-string's ESM code, which import preact in ESM, and some deps only export CJS, which requires preact in CJS. That's a bit annoying, and I'm not sure how this can be fixed. Either solutions so far isn't ideal for DX.

bluwy commented 2 years ago

@mayank99 I added docs in #4267 which hopefully clarifies things a bit. From the repro here and at #4093. The build is still failing as:

  1. This repro: @react-aria/datepicker is incorrectly importing @internationailized/string when specifying vite.ssr.noExternal: ['react-aria', 'react-stately', '@react-stately/*', '@react-aria/*']

    error   Named export 'LocalizedStringDictionary' not found. The requested module '@internationalized/string' is a CommonJS module, which may not support all module.exports as named exports.
    CommonJS modules can always be imported via the default export, for example using:
    
    import pkg from '@internationalized/string';
    const { LocalizedStringFormatter, LocalizedStringDictionary } = pkg;
  2. #4093 repro: downshift is incorrectly exporting ESM. https://publint.bjornlu.com/downshift@6.1.9

  3. #4093 repro: When specifying vite.ssr.noExternal: ['downshift'], a bug in Vite occurs which I'll be fixing separately.

 error   Package subpath './helpers/esm/objectWithoutPropertiesLoose.js' is not defined by "exports" in /Users/bjorn/Downloads/github-bltmyz-5rgtvb/node_modules/@babel/runtime/package.json imported from /Users/bjorn/Downloads/github-bltmyz-5rgtvb/dist/entry.mjs
mayank99 commented 2 years ago

@bluwy Thank you so much for looking into this. Do you think from this point on, these issues should be reported to the respective packages? Since they seem to be using incorrect imports

bluwy commented 2 years ago

Yeah the packages above are built for bundlers only, and not very friendly in nodejs, so it may be worth opening an issue on the respective packages. Those should be out of scope of Astro/Vite as it's a nodejs behaviour.

igorbt commented 2 years ago

I had the same issue trying to use Material UI (MUI) components. It seems like they have the same issue with incorrectly exporting ESM (https://publint.bjornlu.com/@mui/material@5.10.0) and have a Draft PR to fix this, but probably in the next major (https://github.com/mui/material-ui/pull/30510). A workaround that worked for me (I use pnpm) was to add this in package.json:

"pnpm": {
    "overrides": {
      "react": "npm:@preact/compat",
      "react-dom": "npm:@preact/compat"
    }
  }

and shamefully-hoist=true in .npmrc. I use pnpm workspaces so I did this at root level, not in the package with astro app. Hope it helps someone.

Later edit: Actually for some reason this workaround only works for astro dev. When running astro build it displays the true error: '...../@mui/material/Button' is not supported resolving ES modules. So while the ESM export is not fixed in MUI, need to use client:only🤦🏻.

amirrezaDev1378 commented 7 months ago

hello, I am still experiencing this issue on Astro 4.2.8, is this an issue with preact-compat?