feat(legacy): build file name optimization #15115

jiadesen · 2023-11-23T04:00:46Z

Description

Additional context

The default value of the parameter defaultFileName of the getLegacyOutputFileName method has been changed:
[name]-legacy-[hash].js -> [name]-[hash]-legacy.js

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, especially the Pull Request Guidelines.
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).
Update the corresponding documentation if needed.
Ideally, include relevant tests that fail without this PR but pass with it.

jiadesen · 2023-11-23T04:28:01Z

@patak-dev This change breaks some test cases, if you accept it I will maintain them

bluwy · 2023-11-23T07:23:55Z

I don't think this is correct. We should only fix for [hash].js, which gets turned into [hash]-legacy.js. Or legacy-[hash].js

jiadesen · 2023-11-23T07:45:51Z

@bluwy There is a question why the 'legacy' in the variable 'defaultFileName' should be placed between '[name]' and '[hash]'?

I think it is also appropriate to splice legacy in front of .[ext] or .min.[ext]

bluwy · 2023-11-23T08:02:11Z

Because -legacy is an extension of [name]. When you have a chunk named foo, the legacy variant is foo-legacy. In the case where there's no [name], we need to guess still where it looks more correct. If it's closer to the [name] case when it exists, the better and more consistent the output will be.

packages/plugin-legacy/src/index.ts

patak-dev · 2023-11-23T08:23:13Z

This one custom[hash].[format] -> custom-legacy[hash].[format] could also look a bit strange depending on what custom is but I don't think it is bad enough to justify changing it.

About releasing the change to remove the leading - for [hash]xxx patterns, have we changed these output file names in a plugin legacy minor before?

This reverts commit 4ab2036.

This reverts commit 8ee38dd.

This reverts commit 1b50187.

jiadesen · 2023-11-23T09:00:50Z

we need to guess still where it looks more correct.

@patak-dev @bluwy This is a bit difficult, there are too many edge cases, and it is easy to miss and cause errors.

jiadesen · 2023-11-23T09:04:44Z

How about returning to the v4 strategy? Except for the situation where [name] clearly exists, don’t guess about other situations.

patak-dev · 2023-11-23T09:38:19Z

The previous strategy could also give unwanted results and it isn't aligned with the way it normally works when there is a [name] now.

Could you list other edge cases you found?

The one I can think of is (same for any other non word or digit separator):

custom_[hash].[ext] -> custom_-template[hash]

So it may be good to also support this one

Maybe for the [hash] branch, we could do:

if (fileName.startsWith('[hash]')) {
  fileName = `legacy-${fileName}`
}
else if (fileName.includes('/[hash]')) {
  fileName = fileName.replace('/[hash]', '/legacy-[hash]')
}
else {
  // abcd[hash]  -> abcd-legacy[hash]
  // abcd-[hash] -> abcd-legacy-[hash]
  // abcd.[hash] -> abcd-legacy.[hash]
  // abcd_[hash] -> abcd-legacy_[hash]
  fileName = fileName.replace(/[^\w\d]?\[hash\]/, '-legacy$&')
}

jiadesen · 2023-11-23T09:46:03Z

@patak-dev Thanks, I'll try it later

jiadesen · 2023-11-24T04:43:54Z

@patak-dev @bluwy I've done and should work fine

patak-dev · 2023-11-24T08:10:05Z

packages/plugin-legacy/src/index.ts

+          const fileArr = file.split('/')
+          let fileName = fileArr.pop()!


Reading rollup docs, they only mention forward slashes, so I think this should be good and a user passing windows style paths here is not supported https://rollupjs.org/configuration-options/#output-assetfilenames

What is the reason to only apply the conversion to the file part? I guess /foo/[name]/[hash].[ext] is valid.

Oh... I totally overlooked that. Thanks for reviewing this sapphi. I guess we need to go with something like I had at #15115 (comment)

What is the reason to only apply the conversion to the file part? I guess /foo/[name]/[hash].[ext] is valid.

Although rollup allows this use, there is no mention of this usage in all related documentation and the default values of the properties, and these placeholders: [name], [hash], [ext], [extname] are strictly speaking only can be used for the final file name.

patak-dev · 2023-11-24T08:11:38Z

packages/plugin-legacy/src/index.ts

+              // custom[hash].[format] -> [name]-legacy[hash].[format]
+              // custom-[hash].[format] -> [name]-legacy-[hash].[format]
+              // custom.[hash].[format] -> [name]-legacy.[hash].[format]
+              fileName = fileName.replace(/[.-]?\[hash\]/, '-legacy$&')


I think we should support other separators here (using [^\w\d] instead of [.-]), but I'll send another PR to do that, so we keep this one strictly about improving leading [hash] in the pattern.

patak-dev · 2023-11-24T08:13:59Z

packages/plugin-legacy/src/index.ts

+              // entry.min.js -> entry-legacy.min.js
+              // [hash].[format] -> [hash]-legacy.[format]
+              // [hash]custom.[format] -> [hash]custom-legacy.[format]
+              fileName = fileName.replace(/(.+?)\.(.+)/, '$1-legacy.$2')


I was proposing we use legacy-[hash] instead, but if we are trying to keep the legacy files as close as possible to their modern counterparts, then having -legacy at the end is better here.

if we are trying to keep the legacy files as close as possible to their modern counterparts, then having -legacy at the end is better here.

I think that doesn't happen because [hash] is different for modern chunks and legacy chunks.

Ah... then legacy-[hash] would be better here, no? At least there is some kind of order, with different hashes then all legacy and modern chunks will end up intermixed.

Yeah, I think legacy-[hash] would be better. I wonder if we should just use [hash] in that case though.

If we do that, shouldn't we consider not adding legacy for every case when there is a [hash]? I think it is still useful to have legacy because you can't know what files are modern or legacy by looking only at the file name if not, no? (and the order will still be all mixed)

If a user set it to [hash], I guess the user doesn't want to give any information from the file name or wants to reduce the filename length. So in that case, the user wants the filename without legacy.

If it is about not showing any info about the filename, then it doesn't hurt to have legacy-. The filename length is interesting. I wonder if we should let the users configure the names to override our defaults. I'm starting to think that we're crossing a limit on the complexity we are adding here, and maybe it would be better to have a clear rule for everything like adding legacy- to the basename in all cases (or maybe place all the legacy files in legacy/ with the same name, the problem there may be that the file names could be confused when opening them in editors that only show the basename).

If we are going further, I think it would be better to make users possible to pass the names like [name][legacy:-]-[hash].[format]. But for now, I think going with the current PR is fine. If someone requests a more fine-grained control, we can ask them to implement that.

jiadesen · 2023-11-24T09:16:14Z

@patak-dev
If the legacy logo must be spliced, it will be difficult to achieve perfection. There will always be reasons to challenge the splicing position from various angles. In fact, according to my original idea, splicing legacy in .[ext] or . min.[ext] before, because I only divided fileName into two parts: [custom].[min.ext] (min is optional), it does not destroy the integrity of [custom] and simplifies the code.

sapphi-red · 2023-11-24T16:08:20Z

packages/plugin-legacy/src/index.ts

+          const fileArr = file.split('/')
+          let fileName = fileArr.pop()!


What is the reason to only apply the conversion to the file part? I guess /foo/[name]/[hash].[ext] is valid.

sapphi-red · 2023-11-24T16:11:17Z

packages/plugin-legacy/src/index.ts

+              // entry.min.js -> entry-legacy.min.js
+              // [hash].[format] -> [hash]-legacy.[format]
+              // [hash]custom.[format] -> [hash]custom-legacy.[format]
+              fileName = fileName.replace(/(.+?)\.(.+)/, '$1-legacy.$2')


if we are trying to keep the legacy files as close as possible to their modern counterparts, then having -legacy at the end is better here.

I think that doesn't happen because [hash] is different for modern chunks and legacy chunks.

packages/plugin-legacy/src/index.ts

see #15115 (comment)

sapphi-red · 2023-11-24T16:19:32Z

About releasing the change to remove the leading - for [hash]xxx patterns, have we changed these output file names in a plugin legacy minor before?

There's only two PR that changed the output file names in plugin legacy: #11599 (5.0.0), #4122 (1.4.4)
I feel the second one is more a bug fix than a breaking change. So I think we only changed in a major version.

sapphi-red · 2023-11-24T16:34:53Z

Could you list other edge cases you found?

Another edge case is when hash length is specified (e.g. [hash:10]).

…_optimization

jiadesen · 2023-11-28T04:41:52Z

@patak-dev @sapphi-red

The latest changes only include the following two points:

The threshold for entering the [hash] branch has been raised: there needs to be at least one character in front of the [hash] and it cannot be immediately adjacent to /
Compatible with [hash:10]

ps:

I don't think it's important to have too much discussion here. Not many people really care about the product name, and probably even fewer people care about the splicing position of legacy.

If must pay attention to splicing position, then the best advice I can provide is to always splice legacy before .[ext] or .min.[ext].

No matter how we make changes here in the future, can we solve the initial strange problem now?
assets/[hash].[ext] -> assets/-legacy[hash].[ext]

patak-dev · 2023-11-28T08:11:09Z

We should extract these regexes to be reused before merging. To me, I think it is ok to move forward with these two changes and merge them in the next plugin-legacy minor.

jiadesen · 2023-11-28T08:52:21Z

We should extract these regexes to be reused before merging.

@patak-dev Which file should the regexes be extracted to? To snippets.ts?

patak-dev · 2023-11-28T08:55:19Z

@jiadesen they can be extracted to the same index.ts file after the imports. Example https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/css.ts#L162

jiadesen · 2023-11-28T10:27:11Z

@patak-dev Like this? 😂

patak-dev · 2023-11-28T10:58:36Z

No, you can extract both regex as is. It is about avoiding the creation of new regexes on each check, and not about reusing that part of the regex.

const nonLeadingHashInFileNameRE = /[^/]+\[hash(?::\d+)?\]/
const prefixedHashInFileNameRE = /[.-]?\[hash(:\d+)?\]/

jiadesen · 2023-11-28T11:30:23Z

@patak-dev done

…_optimization

refactor(plugin-legacy): build product file name optimization

1b50187

jiadesen added 2 commits November 23, 2023 15:17

test(playground/legacy): update case

8ee38dd

style(plugin-legacy): prettier

4ab2036

patak-dev reviewed Nov 23, 2023

View reviewed changes

packages/plugin-legacy/src/index.ts Show resolved Hide resolved

jiadesen added 3 commits November 23, 2023 16:27

Revert "style(plugin-legacy): prettier"

edad9b8

This reverts commit 4ab2036.

Revert "test(playground/legacy): update case"

e19c550

This reverts commit 8ee38dd.

Revert "refactor(plugin-legacy): build product file name optimization"

bdaf9e3

This reverts commit 1b50187.

jiadesen added 2 commits November 24, 2023 12:06

refactor(plugin-legacy): build product file name optimization

735fdeb

chore(plugin-legacy): remove unnecessary features

61fa777

patak-dev reviewed Nov 24, 2023

View reviewed changes

patak-dev previously approved these changes Nov 24, 2023

View reviewed changes

sapphi-red reviewed Nov 24, 2023

View reviewed changes

jiadesen added 2 commits November 25, 2023 15:24

chore(plugin-legacy): simplify if branches

ee3f5f6

chore(plugin-legacy): compatible with [hash:10]

596f6a0

jiadesen added 3 commits November 27, 2023 14:13

Merge branch 'vitejs:main' into plugin-legacy_getLegacyOutputFileName…

e181f62

…_optimization

Merge branch 'vitejs:main' into plugin-legacy_getLegacyOutputFileName…

97ce1d8

…_optimization

refactor(plugin-legacy): build product file name optimization

a8ffc30

patak-dev added p2-nice-to-have Not breaking anything but nice to have (priority) plugin: legacy labels Nov 28, 2023

chore(plugin-legacy): extract regexes

2a99167

Merge branch 'vitejs:main' into plugin-legacy_getLegacyOutputFileName…

a2e47fc

…_optimization

sapphi-red changed the title ~~refactor(plugin-legacy): build product file name optimization~~ feat(-legacy): build product file name optimization Nov 28, 2023

sapphi-red changed the title ~~feat(-legacy): build product file name optimization~~ feat(legacy): build file name optimization Nov 28, 2023

sapphi-red approved these changes Nov 28, 2023

View reviewed changes

patak-dev approved these changes Nov 28, 2023

View reviewed changes

patak-dev merged commit 39f435d into vitejs:main Nov 28, 2023
12 checks passed

jiadesen deleted the plugin-legacy_getLegacyOutputFileName_optimization branch November 28, 2023 12:22

patak-dev mentioned this pull request Nov 28, 2023

feat(legacy): support any separator before hash in fileNames #15170

Merged

4 tasks

		const fileArr = file.split('/')
		let fileName = fileArr.pop()!

feat(legacy): build file name optimization #15115

feat(legacy): build file name optimization #15115

Conversation

jiadesen commented Nov 23, 2023 • edited Loading

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

jiadesen commented Nov 23, 2023 • edited Loading

bluwy commented Nov 23, 2023 • edited Loading

jiadesen commented Nov 23, 2023

bluwy commented Nov 23, 2023

patak-dev commented Nov 23, 2023

jiadesen commented Nov 23, 2023

jiadesen commented Nov 23, 2023

patak-dev commented Nov 23, 2023 • edited Loading

jiadesen commented Nov 23, 2023

jiadesen commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiadesen commented Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapphi-red commented Nov 24, 2023

sapphi-red commented Nov 24, 2023

jiadesen commented Nov 28, 2023

patak-dev commented Nov 28, 2023

jiadesen commented Nov 28, 2023

patak-dev commented Nov 28, 2023

jiadesen commented Nov 28, 2023

patak-dev commented Nov 28, 2023

jiadesen commented Nov 28, 2023

jiadesen commented Nov 23, 2023 •

edited

Loading

jiadesen commented Nov 23, 2023 •

edited

Loading

bluwy commented Nov 23, 2023 •

edited

Loading

patak-dev commented Nov 23, 2023 •

edited

Loading

jiadesen commented Nov 24, 2023 •

edited

Loading