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

[agriffis]

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]

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]

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?

[agriffis]

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]

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

[agriffis]

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]

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]

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

[agriffis]

@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]

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

[agriffis]

@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]

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

[agriffis]

@patak-dev Sure, see #6869