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.

Sign up for GitHub

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(legacy): build file name optimization #15115

Merged
merged 15 commits into from
Nov 28, 2023
Merged

feat(legacy): build file name optimization #15115

Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1b50187
refactor(plugin-legacy): build product file name optimization
jiadesen Nov 23, 2023
8ee38dd
test(playground/legacy): update case
jiadesen Nov 23, 2023
4ab2036
style(plugin-legacy): prettier
jiadesen Nov 23, 2023
edad9b8
Revert "style(plugin-legacy): prettier"
jiadesen Nov 23, 2023
e19c550
Revert "test(playground/legacy): update case"
jiadesen Nov 23, 2023
bdaf9e3
Revert "refactor(plugin-legacy): build product file name optimization"
jiadesen Nov 23, 2023
735fdeb
refactor(plugin-legacy): build product file name optimization
jiadesen Nov 24, 2023
61fa777
chore(plugin-legacy): remove unnecessary features
jiadesen Nov 24, 2023
ee3f5f6
chore(plugin-legacy): simplify if branches
jiadesen Nov 25, 2023
596f6a0
chore(plugin-legacy): compatible with `[hash:10]`
jiadesen Nov 25, 2023
e181f62
Merge branch 'vitejs:main' into plugin-legacy_getLegacyOutputFileName…
jiadesen Nov 27, 2023
97ce1d8
Merge branch 'vitejs:main' into plugin-legacy_getLegacyOutputFileName…
jiadesen Nov 28, 2023
a8ffc30
refactor(plugin-legacy): build product file name optimization
jiadesen Nov 28, 2023
2a99167
chore(plugin-legacy): extract regexes
jiadesen Nov 28, 2023
a2e47fc
Merge branch 'vitejs:main' into plugin-legacy_getLegacyOutputFileName…
jiadesen Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions packages/plugin-legacy/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,24 +331,30 @@ function viteLegacyPlugin(options: Options = {}): Plugin[] {
}

return (chunkInfo) => {
let fileName =
const file =
typeof fileNames === 'function' ? fileNames(chunkInfo) : fileNames
const fileArr = file.split('/')
let fileName = fileArr.pop()!
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if (fileName.includes('[name]')) {
// [name]-[hash].[format] -> [name]-legacy-[hash].[format]
fileName = fileName.replace('[name]', '[name]-legacy')
} else if (fileName.includes('[hash]')) {
// 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$&')
jiadesen marked this conversation as resolved.
Show resolved Hide resolved
} else {
// entry.js -> entry-legacy.js
// entry.min.js -> entry-legacy.min.js
fileName = fileName.replace(/(.+?)\.(.+)/, '$1-legacy.$2')
if (fileName.includes('[hash]') && !fileName.startsWith('[hash]')) {
sapphi-red marked this conversation as resolved.
Show resolved Hide resolved
// 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$&')
Copy link
Member

Choose a reason for hiding this comment

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

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.

} else {
// entry.js -> entry-legacy.js
// 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')
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
}

return fileName
return [...fileArr, fileName].join('/')
}
}

Expand Down