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

fix: don't force terser on non-legacy (fix #6266) #6272

Merged
merged 2 commits into from
Dec 29, 2021

Conversation

agriffis
Copy link
Contributor

Description

This fixes two misbehaviors of the legacy plugin:

  1. Respect {minify: false} for legacy assets.
  2. Don't inflict es2019/terser on non-legacy chunks.

For the first problem, we could have fixed by checking for false in
viteLegacyPlugin.config(). Unfortunately that would have left the second
problem unsolved. Without adding significant complexity to the config,
there's no easy way to use different minifiers in the build depending on
the individual chunk.

So instead we include terserPlugin() whenever minify is enabled, even
true or 'esbuild', then check the actual configuration in the plugin.
This allows the legacy plugin to inject its special override, leaving
all the non-legacy stuff intact and uncomplicated.

Fixes #6266

See also, previous attempts: #5157 #5168


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.

@agriffis
Copy link
Contributor Author

Need to think a bit about compat.

Update vite and legacy plugin, all good.

Update vite with old plugin, that's fine too, just doesn't fix the bug.

Update plugin with old vite, need to handle that.

@patak-dev
Copy link
Member

We should refactor the plugin if we use this approach to avoid requiring terser if it isn't used. But I think there should be a simpler way to move forward. Would it be possible to add an include/exclude filter to the terser plugin and configure it with the file name pattern to avoid applying it where we don't need it?

This fixes two misbehaviors of the legacy plugin:

1. Respect {minify: false} for legacy assets.
2. Don't inflict es2019/terser on non-legacy chunks.

For the first problem, we could have fixed by checking for false in
viteLegacyPlugin.config(). Unfortunately that would have left the second
problem unsolved. Without adding significant complexity to the config,
there's no easy way to use different minifiers in the build depending on
the individual chunk.

So instead we include terserPlugin() whenever minify is enabled, even
true or 'esbuild', then check the actual configuration in the plugin.
This allows the legacy plugin to inject its special override, leaving
all the non-legacy stuff intact and uncomplicated.

See also, previous attempts: vitejs#5157 vitejs#5168
@agriffis
Copy link
Contributor Author

agriffis commented Dec 27, 2021

So the only compat issue is updated plugin with old vite. Because the plugin no longer forces terser in the config, the legacy chunks won't get built correctly. Best solution is to bump the peer dep. We're in good company since plugin-vue has bumped peer deps a few times, so it seems to be a reasonable approach.

@agriffis
Copy link
Contributor Author

I folded in that change and force-pushed because I wanted to update the commit message slightly anyway

@agriffis
Copy link
Contributor Author

We should refactor the plugin if we use this approach to avoid requiring terser if it isn't used.

I'm not sure what you mean. The terser dep is in the vite package, presumably so it's easy for users to specify a different value for minify without needing to manage additional deps.

Or do you mean terserPlugin() in the plugin stack? That's just going to zip through fast-path if it's not enabled. Is it worth adding complexity to avoid it?

But I think there should be a simpler way to move forward. Would it be possible to add an include/exclude filter to the terser plugin and configure it with the file name pattern to avoid applying it where we don't need it?

If we did this, it would make sense to use the same approach for the existing __vite_skip_esbuild__ hack that the plugin currently employs, since they go hand-in-hand. I like the idea of removing the opts hackery. I'm a little bit uncertain about tying the legacy plugin strongly to a filename pattern, but it might be okay.

@patak-dev
Copy link
Member

We should refactor the plugin if we use this approach to avoid requiring terser if it isn't used.

I'm not sure what you mean. The terser dep is in the vite package, presumably so it's easy for users to specify a different value for minify without needing to manage additional deps.

Or do you mean terserPlugin() in the plugin stack? That's just going to zip through fast-path if it's not enabled. Is it worth adding complexity to avoid it?

I mean that when the plugin is initialized, there is a Worker being created that requires terser. We shouldn't pay for this if it isn't going to be used, no? We could lazily create this worker only if it is used.

But I think there should be a simpler way to move forward. Would it be possible to add an include/exclude filter to the terser plugin and configure it with the file name pattern to avoid applying it where we don't need it?

If we did this, it would make sense to use the same approach for the existing __vite_skip_esbuild__ hack that the plugin currently employs, since they go hand-in-hand. I like the idea of removing the opts hackery. I'm a little bit uncertain about tying the legacy plugin strongly to a filename pattern, but it might be okay.

I think you are right, given that the names can also be configured. Checking the code for the terser plugin, all this logic may be starting to be more complex than just extracting the terser functionality into a utility and calling terser directly in the legacy plugin.

I let @sodatea or others check what they think is best here.

@agriffis
Copy link
Contributor Author

agriffis commented Dec 28, 2021

We could lazily create this worker only if it is used.

Ah, right. I added a commit for this. It seems like a good idea anyway!

Checking the code for the terser plugin, all this logic may be starting to be more complex than just extracting the terser functionality into a utility and calling terser directly in the legacy plugin.

This sounds nice in principle, I'm not sure in practice. 😕 The legacy builds need to run through the same stack as the regular builds. The only differences (so far) are (1) don't run esbuild, (2) use terser for minification. If the legacy plugin called terser directly, wouldn't that change the effective source-processing order? (Genuine question, I'm quite new to vite and rollup.)

@patak-dev patak-dev merged commit 1da104e into vitejs:main Dec 29, 2021
poyoho added a commit to poyoho/vite that referenced this pull request Dec 30, 2021
commit d856c4b
Author: Anthony Fu <anthonyfu117@hotmail.com>
Date:   Thu Dec 30 00:25:59 2021 +0800

    fix(ssr): move `vite:ssr-require-hook` after user plugins (vitejs#6306)

commit b45f4ad
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Wed Dec 29 14:49:15 2021 +0100

    chore(deps): update all non-major dependencies (vitejs#6185)

commit 4d75b2e
Author: Niputi <7137178+Niputi@users.noreply.github.com>
Date:   Wed Dec 29 14:48:13 2021 +0100

    feat: catch postcss error messages (vitejs#6293)

commit f68ed8b
Author: Bogdan Chadkin <trysound@yandex.ru>
Date:   Wed Dec 29 16:40:13 2021 +0300

    fix: replace execa with cross-spawn (vitejs#6299)

commit 44bb4da
Author: patak <matias.capeletto@gmail.com>
Date:   Wed Dec 29 12:51:46 2021 +0100

    chore(deps): update to esbuild fixed at 0.14.3 (vitejs#5861)

commit 9ad7c55
Author: patak <matias.capeletto@gmail.com>
Date:   Wed Dec 29 11:32:49 2021 +0100

    deps: update to typescript 4.5.4 (vitejs#6297)

commit 1da104e
Author: Aron Griffis <aron@scampersand.com>
Date:   Wed Dec 29 02:50:19 2021 -0500

    fix: don't force terser on non-legacy (fix vitejs#6266) (vitejs#6272)

commit 5279de6
Author: ygj6 <7699524+ygj6@users.noreply.github.com>
Date:   Wed Dec 29 05:30:47 2021 +0800

    feat: import.meta.glob support ?raw (vitejs#5545)

commit 6d4ee18
Author: Bjorn Lu <bjornlu.dev@gmail.com>
Date:   Wed Dec 29 04:27:49 2021 +0800

    feat(define): prevent assignment (vitejs#5515)

commit ac3f434
Author: Bogdan Chadkin <trysound@yandex.ru>
Date:   Tue Dec 28 23:26:46 2021 +0300

    fix: upgrade postcss-modules (vitejs#6248)

commit 5a111ce
Author: Bogdan Chadkin <trysound@yandex.ru>
Date:   Tue Dec 28 23:23:42 2021 +0300

    fix: replace chalk with picocolors (vitejs#6277)

commit 7e3e84e
Author: patak-dev <matias.capeletto@gmail.com>
Date:   Tue Dec 28 15:07:10 2021 +0100

    release: v2.7.9

commit 83ad7bf
Author: Anthony Fu <anthonyfu117@hotmail.com>
Date:   Tue Dec 28 21:52:41 2021 +0800

     fix: revert vitejs#6251 (vitejs#6290)

    This reverts commit 49da986.

commit 1cbf0e1
Author: Cristian Pallarés <cristian@pallares.io>
Date:   Tue Dec 28 11:30:42 2021 +0100

    test: fix test typo (vitejs#6285)

commit d13ced5
Author: patak-dev <matias.capeletto@gmail.com>
Date:   Tue Dec 28 09:40:48 2021 +0100

    release: v2.7.8

commit dcb1df4
Author: itibbers <jxn2014@gmail.com>
Date:   Tue Dec 28 16:30:32 2021 +0800

    docs: add frontmatters to fix __VP_STATIC_START__ (vitejs#6283)

commit 60ce7f9
Author: Anthony Fu <anthonyfu117@hotmail.com>
Date:   Tue Dec 28 16:20:12 2021 +0800

    fix(ssr): capture scope declaration correctly (vitejs#6281)

commit eb08ec5
Author: Niputi <7137178+Niputi@users.noreply.github.com>
Date:   Tue Dec 28 06:10:37 2021 +0100

    chore: remove acorn plugins (vitejs#6275)

commit 64b1595
Author: zhangenming <282126346@qq.com>
Date:   Tue Dec 28 11:52:51 2021 +0800

    chore(create-vite): add more gitignore (vitejs#6247)

commit 49da986
Author: sanyuan <39261479+sanyuan0704@users.noreply.github.com>
Date:   Tue Dec 28 05:30:54 2021 +0800

    fix: seperate source and dep for dymamic import after build (vitejs#6251)

commit 394539c
Author: Bogdan Chadkin <trysound@yandex.ru>
Date:   Tue Dec 28 00:29:23 2021 +0300

    fix: upgrade to launch-editor with picocolors (vitejs#6209)

commit 40e3f73
Author: Shinigami <chrissi92@hotmail.de>
Date:   Mon Dec 27 11:42:24 2021 +0100

    chore: fix link (vitejs#6269)

commit e7306b5
Author: Shinigami <chrissi92@hotmail.de>
Date:   Mon Dec 27 11:22:44 2021 +0100

    chore: update bug report issue template (vitejs#6263)

commit 1f945f6
Author: Aaron Bassett <arbassett4@outlook.com>
Date:   Sun Dec 26 15:28:47 2021 -0500

    fix(html): show error overlay when parsing invalid file (vitejs#6184)

commit 1d722c5
Author: patak-dev <matias.capeletto@gmail.com>
Date:   Sun Dec 26 06:35:04 2021 +0100

    release: v2.7.7

commit 2e3fe59
Author: Anthony Fu <anthonyfu117@hotmail.com>
Date:   Sun Dec 26 13:13:24 2021 +0800

    fix(ssr): transform class props (vitejs#6261)

commit 1a6e2da
Author: ygj6 <7699524+ygj6@users.noreply.github.com>
Date:   Sat Dec 25 18:24:44 2021 +0800

    docs: typescript tips for using Type-Only Imports and Export (vitejs#6260)

commit 6a47083
Author: Haoqun Jiang <haoqunjiang@gmail.com>
Date:   Fri Dec 24 14:02:43 2021 +0800

    fix: update the vue version in the error message (vitejs#6252)

commit 485e298
Author: Anthony Fu <anthonyfu117@hotmail.com>
Date:   Thu Dec 23 21:45:13 2021 +0800

    fix(ssr): nested destucture (vitejs#6249)
@agriffis
Copy link
Contributor Author

@patak-dev Thank you for merging this. Now that vite has been released a couple times since this PR, the peer dep will be wrong whenever it is merged to the release branch. Just a heads up. 😄

@patak-dev
Copy link
Member

I think we shouldn't release the plugin until 2.8 is out, so we can use a minor as the peer dep

@agriffis
Copy link
Contributor Author

@patak-dev Now that 2.8 (and 2.8.1 too!) are released, I think it's time to bump the peer dep on the legacy plugin, right?

@patak-dev
Copy link
Member

Thanks for the heads up! Would you send a PR for that @agriffis?

@agriffis
Copy link
Contributor Author

@patak-dev Sure, see #6869

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

legacy plugin forces minify
5 participants