-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Fetch priority #16991
Fetch priority #16991
Conversation
For maintainers only:
|
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.
Looks good, now we need a test cases
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.
This implementation looks great.
We need test cases for at least two/three scenarios:
- I can use
module.parser.javascript.dynamicImportFetchPriority
and it works correctly w/ high/low/auto - I can not use
module.parser.javascript.dynamicImportFetchPriority
but instead use magic comments/* webpackFetchPriority: "high/low/etc." */
and it works - I can use both features and override at the
import()
level usingwebpackFetchPriority:
and it works correctly.
Here is example test cases you can leverage/learn from:
-
@alexander-akait Is there anywhere else I'm missing?
@TheLarkInn Yeah, right, like we do for other |
@aboktor we merged a few PR's for tomorrow release, and now we have some merge conflicts. Would you mind rebasing, and you can probably move this over to "Ready for review" once you've done so. |
The CI is failing on many of the integration tests. Some of those failures I understand (e.g. ConfigTestCases › async-commons-chunk › existing-name › exported tests › should not have duplicate chunks in blocks), and the rest of them I don't. The problem is, I can't repro these failures locally. I tried running |
@aboktor yeah, I will look at this soon, we will merge it to the next release, don't worry |
Notes for me:
|
@alexander-akait Thanks for your update. I labeled the Pull Request so reviewers will review it again. @TheLarkInn Please review the new changes. |
@alexander-akait can you fix the merge conflicts and I'd also like @aboktor's |
One comment on @alexander-akait's changes: Now the fetch priority is set per chunk at compile time. From what I see in the code, the chunk will pick a random priority (based on execution order of the code in ChunkPrefetchPreloadPlugin.js) from any of the ChunkGroups it belongs to. |
/** | ||
* function to return webpack's Trusted Types policy | ||
* Arguments: () => TrustedTypePolicy | ||
*/ |
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.
Inaccurate comment.
|
Let's sort out this design for next Wednesdays release. Need to handle rebase conflicts in addition to make sure we are on the same page. |
/** @type {ExpressionNode} */ (prop.value) | ||
); | ||
if (expr.isBoolean()) { | ||
groupOptions.fetchPriority = "auto"; |
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.
@aboktor , what is the purpose of allowing a boolean at all, when setting it (and regardless of the value) will result in "auto"
? I would think just keeping it to the three enums -- "high", "low", "auto" -- would keep it both simpler and more understandable
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 believe @alexander-akait added that code. I do see that pattern followed throughout webpack though.
@@ -5579,6 +5579,11 @@ declare interface JavascriptParserOptions { | |||
*/ | |||
createRequire?: string | boolean; | |||
|
|||
/** | |||
* Specifies global fetchPriority for dynamic import. |
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.
@aboktor , I think it would be helpful to note the default value is "auto" (by which I assume it means that we leave it up to the browser's defaults); and also what a boolean value would mean.
(Though like I mentioned in another comment, I'm finding it hard to understand what true
or false
would mean in this case at all; do we need them?)
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 agree, let's remove true
, false
requires to disable it, you can have a parent module with webpackFetchPriority: "high"
and want to disable it for some deep modules, would say it's fine tuning, we use the same logic for any magic comments, also some libraries can use webpackFetchPriority
comment too but you don't want it, so we allow to disable it
Summary
fixes #16989
🤖 Generated by Copilot at 0619c3e
This pull request adds a new feature to webpack that allows users and plugins to control the fetch priority of dynamic imports, which can affect the performance and resource management of chunk loading. It introduces a new option
dynamicImportFetchPriority
in the webpack configuration, and a new propertyfetchPriority
in theChunkGroup
class and the import options object. It also modifies several runtime modules and theLoadScriptRuntimeModule
class to support thefetchPriority
option. Additionally, it adds a.gitattributes
file to enforce consistent line endings for JavaScript files.Details
Adds webpackFetchPriority to magic comments
Adds dynamicImportFetchPriorityto module.parser.javascript
Consumes that and sets ChunkGroup.options.fetchPriority
Passes ChunkGroup.options.fetchPriority down to ensure chunk and co to set the right priority on the script tag
Currently completely untested but looking for early feedback
🤖 Generated by Copilot at 0619c3e
.gitattributes
file to enforce LF line endings for JavaScript files (link)fetchPriority
property forChunkGroup
class to represent the priority of fetching a chunk group dynamically (link)dynamicImportFetchPriority
option to the webpack configuration to specify the global default value for thefetchPriority
property (link, link)webpackFetchPriority
option to the import options object to allow overriding the global default value for thefetchPriority
property for a specific dynamic import (link)fetchPriority
option when generating the code for theensureChunk
function in different runtime modules (RuntimeTemplate
,EnsureChunkRuntimeModule
,JsonpChunkLoadingRuntimeModule
) (link, link, link, link)fetchPriority
attribute on the script tag when loading a chunk vialoadScript
function inLoadScriptRuntimeModule
(link, link)