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(config): esbuild minify configurable #6178

Closed
wants to merge 2 commits into from
Closed

feat(config): esbuild minify configurable #6178

wants to merge 2 commits into from

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Dec 19, 2021

Description

fix #6065

Additional context

esbuild minify configurable


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.

@poyoho poyoho changed the title feat: esbuild minify configurable feat(config): esbuild minify configurable Dec 19, 2021
@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Dec 19, 2021
@ydcjeff
Copy link
Contributor

ydcjeff commented Dec 20, 2021

@poyoho Please change fix: #5619, #6065 to

fix #5619
fix #6065

so that it will close those 2 issues once this PR is merged

@bluwy
Copy link
Member

bluwy commented Dec 23, 2021

I'm not really sure if we want to support this. IMO it deviates from Vite's opinion, and the two issues are trying to solve a very niche edge-case. #5619 seems like it would be better fixed on esbuild side. #6065 is weird that it wants to disable a specific part of minification. I bet it can be solved by refactoring the core problem.

@poyoho
Copy link
Member Author

poyoho commented Dec 23, 2021

yes, #5619 is better fixed on esbuild side. now vite can only set CSS to need minify by default.
but #6065 I guess he wants to configure the parameter of esbuild minify, but vite ignored minify config.userConfig should have a higher priority?

@bluwy bluwy mentioned this pull request Jan 27, 2022
4 tasks
@kskalski
Copy link

kskalski commented Feb 3, 2022

Right now vite.config.js's

  esbuild: {
    minify: false
  }

is ignored and esbuild is still being used for minification, which contradicts documentation at https://vitejs.dev/config/#build-minify
In my opiion user config options from vite.config.js should definitely take priority

@poyoho
Copy link
Member Author

poyoho commented Feb 6, 2022

However, in most cases, the internal configuration of vite has a higher priority.🙈

@poyoho poyoho closed this Feb 6, 2022
@kskalski
Copy link

kskalski commented Mar 9, 2022

However, in most cases, the internal configuration of vite has a higher priority.🙈

Could you clarify if this is current / desired / documented behavior?

@poyoho
Copy link
Member Author

poyoho commented Mar 10, 2022

IIUC, @patak-dev said in my other PR. So I closed this PR because it violated the rules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vite.config.js esbuild options are ignored, e.g. disabling minification or one of its aspects
5 participants