Skip to content

fix(plugin-legacy): use terser when minifier is not specified#5231

Closed
OrekiSH wants to merge 1 commit into
vitejs:mainfrom
OrekiSH:fix-legacy-minifier
Closed

fix(plugin-legacy): use terser when minifier is not specified#5231
OrekiSH wants to merge 1 commit into
vitejs:mainfrom
OrekiSH:fix-legacy-minifier

Conversation

@OrekiSH
Copy link
Copy Markdown
Contributor

@OrekiSH OrekiSH commented Oct 8, 2021

Description

Only use terser as the default minifier when minifier is not specified, minify: false is useful in some cases I think.

Additional context


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.

Comment thread packages/plugin-legacy/index.js
@Shinigami92 Shinigami92 added p3-minor-bug An edge case that only affects very specific usage (priority) plugin: legacy labels Oct 8, 2021
Comment on lines +100 to +102
const { minify } = config.build
if (typeof minify === 'undefined') {
config.build.minify = 'terser'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const { minify } = config.build
if (typeof minify === 'undefined') {
config.build.minify = 'terser'
if (!config.build || config.build.minify === undefined) {
return { build: { minify: 'terser' } }

I think we should use return instead of mutating the config object directly. (also the case that config.build might be undefinded.

Copy link
Copy Markdown
Contributor Author

@OrekiSH OrekiSH Oct 10, 2021

Choose a reason for hiding this comment

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

Well, in factconfig.build has been set to {} if not exists in line 86. And return value in config option is a little confusing for me. It seems like no param reassign, but it actually mutated config globally.🤔

@sapphi-red
Copy link
Copy Markdown
Member

It seems this is covered with #6272.

@patak-cat
Copy link
Copy Markdown
Member

Closing the PR as it covered as stated by @sapphi-red. @OrekiSH let us know if minify: false isn't working as expected for you. And thanks for the PR

@patak-cat patak-cat closed this Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p3-minor-bug An edge case that only affects very specific usage (priority) plugin: legacy

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants