Skip to content

🐛 BUG: Pseudo ES6 third-party module imports fail when building #3174

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

Closed
1 task done
martypdx opened this issue Apr 21, 2022 · 8 comments
Closed
1 task done

🐛 BUG: Pseudo ES6 third-party module imports fail when building #3174

martypdx opened this issue Apr 21, 2022 · 8 comments
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)

Comments

@martypdx
Copy link
Contributor

martypdx commented Apr 21, 2022

What version of astro are you using?

1.0.0-beta.17

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

None

What package manager are you using?

npm

What operating system are you using?

Mac

Describe the Bug

During astro build, third-party ecosystem modules that present as CJS modules under npm,
but typically have rollup build "module": "dist/builder-react.es5.js", type pseudo-ES6 module presentation that
works under vite, webpack, nextJS, etc. will fail. This section in esbuild docs does a good job explaining the history

I have put together a minimal reproduction repo linked below.

This project will work when run with npm start.

However, when trying to build with npm run build, it throws an error based on the import in
file /src/components/CMS.jsx:

 error   Named export 'Builder' not found. The requested module '@builder.io/react' 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 '@builder.io/react';
  const { Builder, BuilderComponent, builder } = pkg;

If this recommendation is implemented, the code now fails under vite SSR because the imports are undefined.

This used to work in astro, but the change showed up in one of the 0.2x build prior to 1.0.0-beta. IMO, this error needs to be disabled or be configurable to allow these types of modules to be imported.

I'm happy to help with a PR, but would need some direction on 1) what the desired solution is, and 2) where to go in the code

Link to Minimal Reproducible Example

https://github.com/alchemycodelab/astro-module-issue

Participation

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

I did some research on this in hopes of finding a work around.

The source error is coming from this use of node.js' dynamic import. This isn't used in astro dev which only uses vite.

It's importing a prebuilt version of the project in the dist repo from entry.mjs. I'm curious about how the imports get created, as this is what's in that file for the module causing problems:

import * as $$module1 from '@builder.io/react';
import { Builder, BuilderComponent, builder } from '@builder.io/react';

I don't know enough to know why the module is being imported twice.

At this point, it seems I could explore one of two directions:

  1. Modifying the creation of entry.mjs so it has workable imports under node's import.
  2. Change the imports to be straight CJS, then try and modify something with the vite config to get around the fact that CJS breaks vite's import routine

Any suggestions appreciated...

@FredKSchott FredKSchott added bb:investigate - P4: important Violate documented behavior or significantly impacts performance (priority) and removed bb:investigate labels May 10, 2022
@FredKSchott
Copy link
Member

Thanks @martypdx! @matthewp has been chatting about getting rid of this temp file system that we currently use, but I think this would also require a replacement of the import() method as well. More investigation needed, thanks for filing!

@FredKSchott
Copy link
Member

Update: This can be fixed by using vite.ssr.noExternal: ['@builder.io/react']. Similar to #3338 we should have a better error message here / better user experience that guides you towards that fix. Keeping this issue open to reflect that need.

@FredKSchott FredKSchott self-assigned this May 12, 2022
@natemoo-re
Copy link
Member

Update: This can be fixed by using vite.ssr.noExternal: ['@builder.io/react'].

This might not be fixed by vite.ssr.noExternal, unfortunately. Here's a repro. https://stackblitz.com/edit/github-yxin7b?file=astro.config.mjs

@martypdx
Copy link
Contributor Author

Hey thanks for the update. Also great to see builder.io directly engaged in making this work.

I've created a work-around that has will allow us to go live, but it's a bit janky. We basically have two different pages folders and do a pre-npm step to copy that one to be the pages directory. Once I also realized that getStaticPaths gets run outside of the astro page in own process under node.js rules, that helped with a few things.

What's currently not possible is to do a "build" that runs the Builder components on the client (that is what is required for running it in their CMS). To get around this, we do the separate CMS deployment that runs in astro dev mode. But it works though.

The prod build works fine now and we are achieving the goal of a no-react, pure html, css, and minimal vanilla js production website.

@FredKSchott FredKSchott removed their assignment May 20, 2022
@IanVS
Copy link
Contributor

IanVS commented May 26, 2022

Thanks for the workaround of using ssr.noExternal. I hit this problem with @docsearch/react, and that change solved the issue. I was glad not to have to use the approach in the astro docs, it feels kind of gross.

@matthewp matthewp self-assigned this Jun 1, 2022
@matthewp
Copy link
Contributor

matthewp commented Jun 7, 2022

Unfortunately this package cannot be fixed on our end. The package @builder.io/sdk attempts to dynamically use require at runtime, and since Astro is ESM-only, this can never work no matter what configuration we throw at it.

The package will need to be upgraded to be compatible with ESM. I would suggest using new URL instead of require('url') for parsing URLs. I'm not sure if this alone will make the module compatible, but it's the first thing I ran into.

Additionally I would suggest creating Node compatible ESM modules. In Node you need either "type": "module" in your package.json, or you need to use the .mjs extension. Both the @builderio/react and @builderio/sdk packages use .js extensions with no type property, so Node is never going to recognize them as MJS when you attempt to import them.

You can test compatibility outside of Astro or Vite, just run node node_modules/@builder.io/react/dist/builder-react.es5.js and see if it throws or not. At present it will.

Closing as there's nothing we can do on our end about this.

@philr35
Copy link

philr35 commented Mar 27, 2023

I also resolved this issue by using the mentioned vite.ssr.noExternal in my astro config. Thanks!

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