-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
feat(legacy): build file name optimization #15115
feat(legacy): build file name optimization #15115
Conversation
@patak-dev This change breaks some test cases, if you accept it I will maintain them |
I don't think this is correct. We should only fix for |
@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 |
Because |
This one About releasing the change to remove the leading |
@patak-dev @bluwy This is a bit difficult, there are too many edge cases, and it is easy to miss and cause errors. |
How about returning to the v4 strategy? Except for the situation where |
The previous strategy could also give unwanted results and it isn't aligned with the way it normally works when there is a 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 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$&')
} |
@patak-dev Thanks, I'll try it later |
@patak-dev @bluwy I've done and should work fine |
packages/plugin-legacy/src/index.ts
Outdated
const fileArr = file.split('/') | ||
let fileName = fileArr.pop()! |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
packages/plugin-legacy/src/index.ts
Outdated
// 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$&') |
There was a problem hiding this comment.
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.
packages/plugin-legacy/src/index.ts
Outdated
// 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') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
@patak-dev |
packages/plugin-legacy/src/index.ts
Outdated
const fileArr = file.split('/') | ||
let fileName = fileArr.pop()! |
There was a problem hiding this comment.
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.
packages/plugin-legacy/src/index.ts
Outdated
// 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') |
There was a problem hiding this comment.
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.
There's only two PR that changed the output file names in plugin legacy: #11599 (5.0.0), #4122 (1.4.4) |
Another edge case is when hash length is specified (e.g. |
The latest changes only include the following two points:
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 If must pay attention to splicing position, then the best advice I can provide is to always splice No matter how we make changes here in the future, can we solve the initial strange problem now? |
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. |
@patak-dev Which file should the regexes be extracted to? To |
@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 |
@patak-dev Like this? 😂 |
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+)?\]/ |
@patak-dev done |
Description
#15106
Additional context
The default value of the parameter
defaultFileName
of thegetLegacyOutputFileName
method has been changed:[name]-legacy-[hash].js
->[name]-[hash]-legacy.js
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).