-
Notifications
You must be signed in to change notification settings - Fork 5
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
Chunks preload options #7
Conversation
Hey, @mentaljam! |
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.
Hi,
Here are some of my thoughts on the proposed changes.
src/index.ts
Outdated
const {name} = path.parse(cur.fileName) | ||
if (cur.isEntry) { | ||
prev.entries[name] = cur.name | ||
} else if (cur.isDynamicEntry || preload.has(cur.name)) { |
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 just can omit the cur.isDynamicEntry
check and pass all chunks that are not entries to something like prev.otherChunks
. On the other hand we can omit the cur.isDynamicEntry
and leave the preload check and pass them to something like cur.preloadedEntries
. The idea is to prevent double check of preloaded entries.
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.
Is second case the line 374 becomes
} else if (name in preloadedEntries) {
src/types.ts
Outdated
@@ -153,7 +156,7 @@ interface IPluginOptions { | |||
* ['lib'] | |||
* ``` | |||
*/ | |||
preload?: string[] | Set<string> | |||
preload?: PreloadChunk[] |
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 it's better to use
Record<string, 'module' | 'preload' | 'modulepreload'>
Where record keys are names of chunks. It would simplify further checks.
src/index.ts
Outdated
if (linkType) { | ||
addNewLine(head) | ||
head.appendChild(new HTMLElement('link', {}, `rel="preload" href="${filePath}" as="${linkType}"`)) | ||
} else if (name in dynamicEntries) { |
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 use a record for preload
(see below) then the check would be like
} else if (name in dynamicEntries && name in preload) {
According to my comments to the previous commit this check could be moved to the bundle reducing stage.
src/index.ts
Outdated
const chunkShouldBePreloaded = preload.find(x => x.name === name); | ||
if (chunkShouldBePreloaded) { | ||
const linkType = extensionToType(injectType) | ||
if (linkType) { | ||
addNewLine(head) | ||
head.appendChild(new HTMLElement('link', {}, `rel="${chunkShouldBePreloaded.type}" href="${filePath}" as="${linkType}"`)) | ||
} |
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 transforms to
const linkType = extensionToType(injectType)
if (linkType) {
addNewLine(head)
head.appendChild(new HTMLElement('link', {}, `rel="${preloaded[name]}" href="${filePath}" as="${linkType}"`))
}
src/index.ts
Outdated
@@ -90,7 +79,7 @@ function getEntries(files: (OutputAsset | OutputChunk)[], preload: Set<string>) | |||
const {name} = path.parse(cur.fileName) | |||
if (cur.isEntry) { | |||
prev.entries[name] = cur.name | |||
} else if (cur.isDynamicEntry || preload.has(cur.name)) { | |||
} else if (cur.isDynamicEntry || preload.find(x => x.name === cur.name)) { |
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.
According to my comments to the previous commit and comments below to this one, this line could look like
} else if (cur.name in preload) {
Ok. @mentaljam, could you add config for your formatter/prettier? There is no configuration, but my local prettier does everything in its own way |
Sorry, I cannot maintain this package anymore and look over all your commits. If you are interested, I can transfer the ownership of this repo and the npm package to you. |
#6