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

Error when a CJS module is imported from ESM module dependencies #3024

Closed
JBusillo opened this issue Apr 17, 2021 · 20 comments
Closed

Error when a CJS module is imported from ESM module dependencies #3024

JBusillo opened this issue Apr 17, 2021 · 20 comments
Labels
p4-important Violate documented behavior or significantly improves performance (priority)

Comments

@JBusillo
Copy link
Contributor

Describe the bug

if a dependency that is an ES module imports a dependency that is a CJS module, the following error is encountered when running with the development server:

Uncaught SyntaxError: The requested module '/dependency2/index.js' does not provide an export named 'default'

Reproduction

Clone https://github.com/JBusillo/vanilla
do an 'npm i' and 'npm run dev'

System Info

System:
    OS: Linux 5.8 Ubuntu 20.04.2 LTS (Focal Fossa)
    CPU: (6) x64 Intel(R) Core(TM) i5-8400 CPU @ 2.80GHz
    Memory: 22.09 GB / 31.04 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 14.16.1 - /usr/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 7.9.0 - /usr/local/bin/npm
  Browsers:
    Brave Browser: 89.1.22.71
    Chrome: 89.0.4389.114
    Firefox: 87.0
  npmPackages:
    vite: ^2.1.5 => 2.1.5 

Used package manager: npm

Additional Notes

I originally encountered this issue with the module https://github.com/IBM/carbon-components-svelte. The repro provided is simpler to demonstrate.

The problem doesn't affect the production build when using the Rollup commonjs plug-in.

One possible solution might be to do an esbuild transform within transformRequest.ts:

...
import esbuild from 'esbuild'
...
// inserted just prior to the return 'if' block
if (file.endsWith("js")) {
    const r = await esbuild.transform(code, { format: "esm" })
    code = r.code
}
...

This transforms all .js files, even those that are already ESM, but esbuild doesn't modify the code in that case. Perhaps the transformation could be an option. Or, there could be another solution altogether.

This issue is possibly related to #2679, but that addresses UMD modules imported at the root level.

@GrygrFlzr GrygrFlzr added bug p4-important Violate documented behavior or significantly improves performance (priority) and removed pending triage labels Apr 17, 2021
@eirslett
Copy link
Contributor

eirslett commented Apr 17, 2021

I encountered this bug as well - a dependency (Storybook, in my case) imports core-js, and core-js uses CJS internally.
Looks like you can force the file through Esbuild from your Vite config - you add the file to the optimizeDeps "include" array. But that's cumbersome if there are lots of CJS files.

@benmccann
Copy link
Collaborator

benmccann commented Apr 22, 2021

Just FYI, carbon-components-svelte no longer depends on clipboard-copy so this doesn't happen with that module anymore. If you'd like to reproduce this error with it make sure to use the old version: npm install --save carbon-components-svelte@0.38.2

But back when this was reported:

  • the CJS component clipboard-copy is imported by a .svelte file. I wonder if Vite's automatic dependency discovery via crawling only works with JS files. I wouldn't be surprised if it doesn't know how to handle .svelte files. Though I'd hope that it'd do the dependency discovery by resolving through the plugins and that vite-plugin-svelte would be invoked during the crawling. You can make it work by explicitly telling it about this dependency: optimizeDeps: { include: ['clipboard-copy'] }. CC'ing my Svelte partners in crime @dominikg @GrygrFlzr
  • I don't know why it's encountering clipboard-copy in the first place. If I import Accordion from carbon-components-svelte then Vite somehow finds clipboard-copy and chokes, but clipboard-copy is not used by that component and should be completely tree-shaken out, so it's a little frustrating to have it fail trying to instantiate a module that's never used since it unnecessarily breaks things and would be a performance hit even if it did work. If someone's able to take the time to reproduce this using just Vite and not SvelteKit it'd be a good issue to file. Or if vite-plugin-svelte is somehow interfering with tree shaking that'd also be a good issue to file

@ecstrema
Copy link

See this post for a (partial) workaround.

@ygj6
Copy link
Member

ygj6 commented Jun 12, 2021

@benmccann
Copy link
Collaborator

It seems that the main cause of this issue is that Vite skips optimizing non-JS files such as .svelte, .marko, etc. as described in #3910. After some fixes to SvelteKit, I can see CJS dependencies being pre-bundled. However, the import statements in .svelte files are not updated to point to the pre-bundled version, so the code still tries to load the CJS version

@benmccann
Copy link
Collaborator

benmccann commented Aug 9, 2021

Also, what is this about? In the carbon-components-svelte example from above, clipboard-copy isn't being crawled or pre-bundled because it's a dependency. I don't understand why dependencies would be skipped. That seems like the wrong thing to do

// dependency or forced included, externalize and stop crawling

@benmccann benmccann mentioned this issue Aug 9, 2021
9 tasks
@bluwy
Copy link
Member

bluwy commented Aug 10, 2021

@benmccann I've already put some of my thoughts on it in discord, but I thought to write it out here for others.

Regarding that line, what Vite's prebundling essentially does it that /node_modules/my-library prebundles to /node_modules/.vite/my-library.js. So during dependency crawling/import scan, when Vite finds eg import foo from 'my-library' in user code, which is resolved to eg /node_modules/my-library/index.js, Vite stops there as it has reached it's goal (as mentioned in first sentence).

This also means that nested dependencies of my-library won't be detected since Vite doesn't crawl into them (it has reached its goal). This works well for a lot of pure JS library, but for Svelte and Marko where component libraries publish raw uncompiled files, this isn't as helpful anymore.

Though I also don't think continuing the crawl is the solution (?) since it would needlessly impact pure JS libraries, and pnpm users would need to set shamefully-hoist=true (slightly breaking). Unfortunately, I haven't thought of another bullet-proof solution either.

@Shinigami92
Copy link
Member

Discussion notes from todays meeting:

  • CJS is only supported if it's prebundled
  • if it's not in node_modules it does not get optimized

@ljharb
Copy link

ljharb commented Aug 13, 2021

When would CJS packages be prebundled? Bundling is something the app does - very few packages do. (or is “prebundling” a vite config thing)

@Shinigami92
Copy link
Member

I assume it has something to do with this: https://vitejs.dev/guide/dep-pre-bundling.html#the-why

@bluwy
Copy link
Member

bluwy commented Aug 17, 2021

Note: The repro provided uses local packages which are implicitly not optimized by default, which means the CJS module isn't optimized and leads to the error.

@benmccann
Copy link
Collaborator

And by local I think it's meant symlinked

@bluwy
Copy link
Member

bluwy commented Aug 29, 2021

To rephrase the issue, I think it should be: "Error when a CJS module is imported from an unoptimized ESM dependency", since this works today if that ESM dependency is optimized, e.g. optimizeDeps.include: ['esm-dep'].

If the ESM dependency needs to be excluded from optimization, there's a pending PR #4634 that will allow the nested CJS module to be re-included back into optimization, e.g. optimizeDeps.include: ['esm-dep > cjs-dep']. Please try the pending PR out to make sure it works!

@bluwy
Copy link
Member

bluwy commented Sep 1, 2021

According to #3024 (comment):

Discussion notes from todays meeting:

  • CJS is only supported if it's prebundled
  • if it's not in node_modules it does not get optimized

I think we can close this issue as intended behaviour, and also because there's now a way to optimize the nested CJS module via optimizeDeps.include: ['esm-dep > cjs-dep'] IF the esm-dep is excluded from optimization. The feature is supported since Vite v2.5.3.

@ecstrema
Copy link

ecstrema commented Sep 1, 2021

Then, what about "You are trying to import unbundled CommonJS from module {ModuleName}. Please --Insert here the requirements for the code to be imported or offer steps to correct the problem--"
The current error report is quite verbose and unreadable.

@bluwy
Copy link
Member

bluwy commented Sep 2, 2021

I'm not sure if there's enough heuristic to have Vite suggest that error, especially since the error happens on the client side. Though I'm not too familiar with the internals to know if it's possible either. Perhaps you have some ideas where this can be handled?

@ecstrema
Copy link

ecstrema commented Sep 2, 2021

Perhaps you have some ideas where this can be handled?

I unfortunately don't.

@vladkens
Copy link

@bluwy, try this please:
#4209 (comment)

@benmccann
Copy link
Collaborator

There's now an experimental option to prebundle .svelte files: https://github.com/sveltejs/vite-plugin-svelte/blob/main/docs/config.md#prebundlesveltelibraries

@bluwy
Copy link
Member

bluwy commented Feb 26, 2022

Closing this as this is expected behaviour as discussed above:

Discussion notes from todays meeting:

  • CJS is only supported if it's prebundled
  • if it's not in node_modules it does not get optimized

To fix it, optimize the ESM dependency, or the CJS dependency if the parent ESM dep is not optimizable. For example, optimizeDeps.include: ["esm-dep"] or optimizeDeps.include: ["esm-dep > cjs-dep"].

I've looked into providing a friendlier error message, but it's not possible given the browser explicitly request it through import, and there's no way for Vite to intercept and know the intention of the import.

For now this should be a rare error as vite-plugin-svelte and SvelteKit correctly handles pre-bundling now.

@bluwy bluwy closed this as completed Feb 26, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Mar 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

No branches or pull requests

10 participants