Skip to content

feat(optimizer): nested optimization#4634

Merged
patak-cat merged 16 commits into
vitejs:mainfrom
bluwy:feat/nested-optimize
Aug 31, 2021
Merged

feat(optimizer): nested optimization#4634
patak-cat merged 16 commits into
vitejs:mainfrom
bluwy:feat/nested-optimize

Conversation

@bluwy
Copy link
Copy Markdown
Member

@bluwy bluwy commented Aug 16, 2021

Description

Support adding some-lib > nested-lib to optimizeDeps.include. Allowing nested-lib to be optimized if we exclude some-lib from optimization. For example:

// Dependency tree:
some-lib (excluded)
├─ nested-lib (will be included and optimized)

Additional context

  1. some-lib > nested-lib allows any amount of space around the >.
  2. This is part of the work to have svelte libraries "just work". Partially addresses Pre-bundled dependencies doesn't dedupe imports in external files #3910 (comment) (marked p4 important). Further work continued in Automatically add svelte libraries to vite.optimizeDeps.exclude sveltejs/vite-plugin-svelte#125.
  3. This may also help Error when a CJS module is imported from ESM module dependencies #3024 (marked p4 important), allowing a workaround.
  4. In the future, we could do this automatically for all excluded libraries by default? And have optimizeDeps.exclude: ['some-lib/*'] to explicitly exclude the entire tree? Currently, just specifying some-lib will exclude the entire tree.
  5. I also have this repo as sort of a real-world test, It was based on Error when a CJS module is imported from ESM module dependencies #3024. Was using it for the last round of sanity check.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented Aug 17, 2021

Tests fixed. Turns out when linking a cjs package via link:, vite build fails. Using file: instead fixes it so the prod build converts the cjs package to esm.

Shinigami92
Shinigami92 previously approved these changes Aug 17, 2021
Comment thread packages/vite/src/node/plugins/resolve.ts Outdated
Shinigami92
Shinigami92 previously approved these changes Aug 17, 2021
@benmccann
Copy link
Copy Markdown
Collaborator

Should we add some documentation to https://vitejs.dev/guide/dep-pre-bundling.html and https://vitejs.dev/config/#dep-optimization-options so that users know this new option exists?

@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented Aug 18, 2021

@benmccann Yeah I'll update the config docs regarding this feature, looks like there's a cjs warning note for optimizeDeps.exclude that can be updated as well.

I'm not sure this is something we'll want to introduce in the guide though, it's solving a specific edge case, and it feels odd to add a paragraph about this.

@benmccann
Copy link
Copy Markdown
Collaborator

Cool. Nice update on the docs! The other thing I wonder about is if we should come up with a better separator than /node_modules/. E.g. maybe we should use something like :? Using /node_modules/ is deceptively close to a real file path, which might be confusing. For pnpm it almost is a real file path, but for npm it might not make as much sense

Thanks again for this! Really excited about it

@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented Aug 18, 2021

I'm happy to change /node_modules/ to whichever that makes the most sense, it shouldn't be too hard to change in code either. Another idea is with > as it resembles css selectors, but I'll see if the maintainers have opinions of changing this too.

@patak-cat patak-cat added p4-important Violate documented behavior or significantly improves performance (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels Aug 22, 2021
Shinigami92
Shinigami92 previously approved these changes Aug 26, 2021
Copy link
Copy Markdown
Member

@patak-cat patak-cat left a comment

Choose a reason for hiding this comment

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

@bluwy we discussed this with the team today and this is feat is approved. Thanks for pushing for this. There may be a better way to deal with these dependencies in the future (like enabling transpilation of .vue/.svelte/etc files in node_modules) but for the moment this seems like a good tool to enable SvelteKit and others to work around the current issues.

About the separator, your proposal of using the CSS selector sounds better than something that looks like a path. So instead of /node_modules/, something like include: ['foo > bar'] should be familiar enough to users.

@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented Aug 28, 2021

Thanks @patak-js. Excited to get this in! I've updated to use the > separator.

Copy link
Copy Markdown
Member

@patak-cat patak-cat left a comment

Choose a reason for hiding this comment

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

I think we should merge this, and we can keep working to improve on it as people start using it. We could wait until Tuesday though to see if someone else from the reported bugs could try it out and merge it before 2.5.2

About 4, it may be interesting to explore although it is a breaking change. So we may need to add something like

  export default defineConfig({
    optimizeDeps: {
      exclude: ['#esm-dep'] // only main dep, don't exclude subdependencies
    }
  })

It is a bit strange to me that opting out of optimization would still optimize subdeps by default (maybe [ 'esm-dep!' ], hard to find a relatable notation).

@patak-cat patak-cat merged commit f61ec46 into vitejs:main Aug 31, 2021
@bluwy bluwy deleted the feat/nested-optimize branch August 31, 2021 15:30
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
@kalda341
Copy link
Copy Markdown

I'm running a monorepo where I want to exclude dependencies which are packages in the monorepo from prebundling, but include dependencies of those packages. If I'm understanding this correctly then this feature should be perfect for this use case, right?

I've written a script which recursively produces a list of those dependencies in the format described here a > b > c, however this doesn't prevent the "new dependencies found" message and extremely slow start that I was getting before.
I am using PNPM, which I think may be the problem.

I can work around the issue by hoisting (--shamefully-hoist passed to PNPM), and only including dependency by name, not the format described in this PR, but that's not a proper solution.

Am I:

  1. Miss-understanding the use case for this PR?
  2. Encountering a bug with this PR with respect to PNPM?
  3. Doing something else incorrect?

Sorry to hijack an old PR thread, but it seems like the most relevant place to discuss this.

@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented Feb 24, 2022

I'm running a monorepo where I want to exclude dependencies which are packages in the monorepo from prebundling, but include dependencies of those packages. If I'm understanding this correctly then this feature should be perfect for this use case, right?

By default, linked packages in monorepos are excluded by default, and are also scanned for dependencies for inclusion, so you don't have to do those manually. This PR provides a way to re-include nested deps by an excluded dep, e.g. excluded-dep > include-this-dep. If excluded-dep is a linked package, Vite already handles this implicitly.

You shouldn't need shamefully hoist, but judging from your comment about "new dependencies found", it may likely be an issue where Vite doesn't scan fully by default due to the code structure, but it does scan into your linked package.

Perhaps https://github.com/antfu/vite-plugin-optimize-persist would help, but I don't quite know why that happens. You can bring this to a discussion and ping me if you like.

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 improves performance (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants