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

docs: clarify CommonJS with optimizeDeps.exclude #3961

Merged
merged 2 commits into from
Jun 26, 2021

Conversation

aleclarson
Copy link
Member

Description

As a follow-up to #3927, let's clarify that CJS dependencies cannot be added to optimizeDeps.exclude.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@aleclarson aleclarson added the documentation Improvements or additions to documentation label Jun 25, 2021
@benmccann
Copy link
Collaborator

benmccann commented Jun 25, 2021

If an ESM dependency has a CommonJS dependency, it also cannot be excluded.

This has been probably the most commonly hit issue for SvelteKit apps. The issue is that all Svelte components need to be excluded (and SvelteKit in fact searches for these packages and automatically adds them to the ssr.noExternal configuration) because the raw .svelte components must be passed to the Svelte compiler. If the package has already been bundled then the .svelte components have already been converted to .js before we try to build the app, which won't work. #3024 refers to this specifically with a lot of people hitting it in the higher traffic #2579.

Is there a way to exclude a package, but have its dependencies optimized? That would help us tremendously. I'm not sure if there's a hard constraint around it being implemented the way it is now or that's just what was done without a specific need for it

@@ -690,6 +690,10 @@ createServer()

Dependencies to exclude from pre-bundling.

:::warning CommonJS
CommonJS dependencies cannot be excluded from optimization. If an ESM dependency has a CommonJS dependency, it also cannot be excluded.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CommonJS dependencies cannot be excluded from optimization. If an ESM dependency has a CommonJS dependency, it also cannot be excluded.
CommonJS dependencies should not be excluded from optimization. If an ESM dependency has a nested CommonJS dependency, it should not be excluded as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking if we should document to force include the nested dep, eg:

{
  optimizeDeps: {
    exclude: ['esm-dep'],
    include: ['esm-dep-nesed-cjs']
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that really work? If so, let's document it

Copy link
Collaborator

@benmccann benmccann Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that the below at least works as its something I see people doing all the time:

{
  ssr: {
    noExternal: ['svelte-component']
  }
  optimizeDeps: {
    include: ['svelte-component-dep-nested-cjs']
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using optimizeDeps.include on a transitive dependency is fragile and is dependent on the flat node_modules used by npm. When pnpm's default nested structure is used, for example, this breaks because the transitive dependency isn't found:

> Failed to resolve force included dependency: clipboard-copy
Error: Failed to resolve force included dependency: clipboard-copy
    at optimizeDeps

Adding the transitive dependency as a direct dependency appears to avoid this issue, but that seems like it'd cause problems in other ways.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all transitive dependencies be optimized by default? Perhaps the issue here is that Vite's dependency crawling mechanism doesn't work with .svelte files?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this for now. Please create a new thread for the future discussion.

@antfu antfu merged commit 8fa75b5 into vitejs:main Jun 26, 2021
aleclarson added a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
@aleclarson aleclarson deleted the docs/optimizeDeps-exclude branch February 25, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants