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

feat(optimizer): nested optimization #4634

Merged
merged 16 commits into from Aug 31, 2021
Merged

Conversation

bluwy
Copy link
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
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
packages/vite/src/node/plugins/resolve.ts Outdated Show resolved Hide resolved
Shinigami92
Shinigami92 previously approved these changes Aug 17, 2021
@benmccann
Copy link
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
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
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
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-dev patak-dev 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
Member

@patak-dev patak-dev 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
Member Author

bluwy commented Aug 28, 2021

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

Copy link
Member

@patak-dev patak-dev 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).

@kalda341
Copy link

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
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.

None yet

5 participants