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: default build.minify to esbuild #5041

Merged
merged 4 commits into from
Sep 23, 2021
Merged

feat: default build.minify to esbuild #5041

merged 4 commits into from
Sep 23, 2021

Conversation

yyx990803
Copy link
Member

@yyx990803 yyx990803 commented Sep 23, 2021

Caveats

  • esbuild@13 now uses a new installation strategy for installing platform specific binaries, which uses optionalDependencies (details in its changelog). The only downside is that all versions of Yarn will download binaries for all platforms (and eventually only install one), so installing with Yarn will become significantly slower. Not sure if there is a way around this - maybe we should encourage users to use npm/pnpm and avoid Yarn. (And we should probably switch to pnpm in the vite repo itself)

@benmccann
Copy link
Collaborator

+1 to switching to pnpm. It's quite hard to test Vite changes against SvelteKit right now because the latter uses pnpm. If both projects were to use pnpm then we could do pnpm link, which would make life much easier

@yyx990803 yyx990803 added the p4-important Violate documented behavior or significantly improves performance (priority) label Sep 23, 2021
@yyx990803
Copy link
Member Author

Unfortunately our current test setup relies quite heavily on Yarn's node_modules layout. I tried to move to pnpm but many of the tests are breaking due to not being able to resolve correctly when copied to a different temp directory.

@bluwy
Copy link
Member

bluwy commented Sep 23, 2021

Another trouble with pnpm right now is that there's no differentiation between file: and link: protocol. In yarn, they are "copy" and "symlink" operations, while pnpm treats both as "symlink". We would need this separation so we can properly test dependency crawling and optimizaton.

@yyx990803
Copy link
Member Author

I managed to get a decent amount of tests passing under pnpm in a WIP branch: https://github.com/vitejs/vite/tree/pnpm

There are a few tests still failing but I think eventually we should be able to move over.

@dominikg
Copy link
Contributor

dominikg commented Sep 23, 2021

Unfortunately our current test setup relies quite heavily on Yarn's node_modules layout. I tried to move to pnpm but many of the tests are breaking due to not being able to resolve correctly when copied to a different temp directory.

Ran into this aswell when i converted vite-plugin-svelte to pnpm. I resorted to symlinking packages/playground/xxx/node_modules to /temp/serve/xxx/node_modules

https://github.com/sveltejs/vite-plugin-svelte/blob/f112d8b4bb029ca98eed500907d3e79308724eb8/scripts/jestPerTestSetup.ts#L107

iirc vite doesn't use /temp/serve and /temp/build for separate test dirs, but the pattern could still apply

edit: I'm pretty stoked about the pnpm move too. Tops my list of great news for vite this week. ❤️

@dominikg
Copy link
Contributor

Another one for pnpm.. You can use .pnpmfile.cjs to enforce consistent use of workspace dependencies even inside 3rd party dependencies that might be present in playground projects:
https://github.com/sveltejs/vite-plugin-svelte/blob/main/.pnpmfile.cjs

@patak-dev patak-dev merged commit e4892be into main Sep 23, 2021
@ygj6 ygj6 mentioned this pull request Sep 28, 2021
9 tasks
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
* feat: default build.minify to esbuild

* refactor: only enable extra options if minify is true

to avoid alter behavior when using terser

* wip: fix default option check
@antfu antfu deleted the feat/esbuild-minify branch December 24, 2021 05:03
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.

6 participants