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.

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: chunk importmap #16552

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: chunk importmap #16552

wants to merge 7 commits into from

Conversation

bhbs
Copy link
Contributor

@bhbs bhbs commented Apr 29, 2024

It seems that many people have been requesting improvements in chunk cache efficiency, so I've created this PR. Please take a look and review this! Thank you, Vite team!

Description

Fixes: #6773 
PoC: #15373
Discussion: #15372

Creating an importmap for chunks helps prevent cascading cache invalidation. This importmap features a list of filenames with file ID-based hashes linked to filenames with content-based hashes. When one chunk references another, it uses the ID-hashed filename instead of the content-hashed filename. As a result, only the updated chunk requires cache invalidation in the browser, leaving intermediary chunks unchanged. This strategy enhances the cache hit rate following deployments.

Implementation

During the renderChunk step, file hash placeholders in the code are overwritten with stable ID hashes. This process also maintains a record of the hash placeholders and their corresponding ID hashes. (After that, during the augmentHash step, filenames are assigned based on the content.)

Based on the generated chunk list and the record of hash placeholders and their corresponding hashes, an importmap is generated and inserted into the HTML.

image

Others

  • An importmap is generated for legacy bundles using the same method, but a systemjs-importmap is inserted instead of a standard importmap.
  • The importmap is considered during CSS processing (cssPostPlugin).
  • The importmap is considered during import analysis (buildImportAnalysisPlugin).
  • Only one importmap can be loaded at a time (MDN). If an importmap already exists, it needs to be merged (buildHtmlPlugin).

Copy link

stackblitz bot commented Apr 29, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bhbs bhbs marked this pull request as ready for review April 29, 2024 06:59
@bhbs bhbs mentioned this pull request May 2, 2024
@olivierbeaulieu
Copy link

This fixes such a gigantic performance issue in almost every vite app out there - thanks a lot! Hopefully this can get merged soon 👀

@Lgowen
Copy link

Lgowen commented Aug 15, 2024

So this PR has not been merged yet, right?

@paolostyle
Copy link

Is there any way to ask the maintainers to review this? It's a shame that such a useful PR is open for over 4 months without any reviews...

@C-Higgins
Copy link

What is the simplest way to consume this code manually, until (if ever) it gets merged?

@juanespinola05
Copy link

What is the simplest way to consume this code manually, until (if ever) it gets merged?

This ☝

@sapphi-red sapphi-red self-requested a review October 16, 2024 10:44
@sapphi-red sapphi-red added the p3-significant High priority enhancement (priority) label Oct 17, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Sorry that it took time to review.
While I think it needs some small things to polish, the approach looks good to me. I think the remaining tasks are to think about how this works with experimental.renderBuiltUrl and think about the interaction with plugins.

Comment on lines 173 to 179
## html.chunkImportMap

- **Type:** `boolean`
- **Default:** `false`

Whether to inject importmap for generated chunks. This importmap is used to optimize caching efficiency.

Copy link
Member

Choose a reason for hiding this comment

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

It seems build.chunkImportMap is used instead of html.chunkImportMap in the code.
I think build.chunkImportMap is better as I think we should have an option to generate the import map in a separate file so that frameworks and users using backend integration can use it.
Let's move this description to docs/config/build-options.md.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might even be worth being an experimental feature. build.experimentalChunkImportMap

Im really looking forward to this change as itll be a massive win for users.

I expect though at this time there may be gotchas we need to learn and patch, as people report behaviour issues with there individual setups.

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 can set experimental without changing the name. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I need to use this feature as backend integration 👍 . It would be great if we could export ImportMap contents into a file like import-maps.json, and the consumers can name as the build.manifest feature (ref: https://vite.dev/config/build-options.html#build-manifest)

docs/guide/features.md Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/chunkImportMap.ts Outdated Show resolved Hide resolved
Comment on lines +40 to +43
return [
base + augmentFacadeModuleIdHash(output.preliminaryFileName),
base + output.fileName,
]
Copy link
Member

Choose a reason for hiding this comment

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

📝 Note to reviewers: the mapping by the browsers is operated after both the module specifier and the key of import maps are resolved to absolute URLs.
https://html.spec.whatwg.org/multipage/webappapis.html#import-maps:~:text=Such%20remappings%20operate,app.mjs%22

config.root,
normalizePath(filename),
)
const assetsBase = getBaseInHTML(relativeUrlPath, config)
Copy link
Member

Choose a reason for hiding this comment

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

I guess this does not take experimental.renderBuiltUrl into account. For non-runtime ones, we can generate a static import map. But for runtime ones, I guess we would need to output an script that constructs an import map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven’t come up with a good way to use it together with the renderBuiltUrl runtime yet.
Considering that we need to handle the import map from other plugins, we’ll need some kind of clever solution.
Are there any other options that could be used as a reference?

I’ll need some time to investigate and think it over.

Copy link
Member

Choose a reason for hiding this comment

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

I guess there aren't any.

packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved
@@ -519,6 +566,16 @@ function viteLegacyPlugin(options: Options = {}): Plugin[] {
}
}

Copy link
Member

Choose a reason for hiding this comment

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

When build.chunkImportMap is enabled, I guess the application won't work if the browser does not support import maps but supports all features included in detectModernBrowserDetector.
I think we need to add if(HTMLScriptElement&&HTMLScriptElement.supports("importmap"))throw new Error; to detectModernBrowserDetector and modernChunkLegacyGuard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried simply adding an if statement at the end of detectModernBrowserDetector, but it doesn't seem to have worked 👀
It seems that __vite_legacy_guard inside modernChunkLegacyGuard is not being executed from anywhere.
Could you explain how it works?

Copy link
Member

Choose a reason for hiding this comment

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

It seems that __vite_legacy_guard inside modernChunkLegacyGuard is not being executed from anywhere.
Could you explain how it works?

The current modernChunkLegacyGuard uses the fact that browsers errors without executing a script if the script has invalid syntax (unsupported syntax). But in this case, we are judging with runtime code instead of a syntax. So I was suggesting adding the code in modernChunkLegacyGuard (i.e. changing modernChunkLegacyGuard to `if(HTMLScriptElement&&HTMLScriptElement.supports("importmap"))throw new Error;`);export function __vite_legacy_guard(){${detectModernBrowserDetector}};`.

That said, I noticed that the script that has the throw new Error won't run but still the scripts imported by that script would run. Hmm...

Comment on lines +901 to +903
const chunkImportMap = config.build.chunkImportMap
? createChunkImportMap(bundle)
: {}
Copy link
Member

Choose a reason for hiding this comment

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

Probably we need to think about how to let other plugins access this information. I assume other plugins will need to access this information sometimes.

Comment on lines 56 to 72
Object.values(meta.chunks).forEach((chunk) => {
const hashPlaceholder = chunk.fileName.match(hashPlaceholderRE)?.[0]
if (!hashPlaceholder) return
if (hashPlaceholderToFacadeModuleIdHashMap.get(hashPlaceholder)) return

hashPlaceholderToFacadeModuleIdHashMap.set(
hashPlaceholder,
getHash(chunk.facadeModuleId ?? chunk.fileName),
)
})

const codeProcessed = augmentFacadeModuleIdHash(code)
return {
code: codeProcessed,
map: new MagicString(codeProcessed).generateMap({
hires: 'boundary',
}),
Copy link
Member

Choose a reason for hiding this comment

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

📝 For reviewers: This works as rollup calculates the chunk hash after the renderChunk hooks are run since Rollup 3.

Reference: https://www.youtube.com/watch?v=cFwO9UvDzfI

@bhbs
Copy link
Contributor Author

bhbs commented Oct 17, 2024

Thank you for the review ✨
I should have some time this weekend, so I'll incorporate your feedback.

@bhbs
Copy link
Contributor Author

bhbs commented Oct 21, 2024

I’ve pushed what I have for now!
But still these tasks remains:

I think the remaining tasks are to think about how this works with experimental.renderBuiltUrl and think about the interaction with plugins.

I need more time 🧐


### Chunk importmap

Creating an import map for chunks helps prevent the issue of cascading cache invalidation. This import map features a list of stable file IDs linked to filenames with content-based hashes. When one chunk references another, it utilizes the file ID instead of the content-hashed filename. As a result, only the updated chunk needs cache invalidation in the browser, leaving intermediary chunks unchanged. This strategy enhances the cache hit rate following deployments.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Creating an import map for chunks helps prevent the issue of cascading cache invalidation. This import map features a list of stable file IDs linked to filenames with content-based hashes. When one chunk references another, it utilizes the file ID instead of the content-hashed filename. As a result, only the updated chunk needs cache invalidation in the browser, leaving intermediary chunks unchanged. This strategy enhances the cache hit rate following deployments.
Creating an import map for chunks helps prevent the issue of cascading cache invalidation. This import map features a list of stable file IDs linked to filenames with content-based hashes. When one chunk references another, it utilizes the file ID instead of the content-hashed filename. As a result, only the updated chunk needs cache invalidation in the browser, leaving intermediary chunks unchanged. This strategy enhances the cache hit rate following deployments.
To enable this feature, set [`build.chunkImportMap`](/config/build-options.md#build-chunkimportmap) to `true`.

Comment on lines 173 to 179
## html.chunkImportMap

- **Type:** `boolean`
- **Default:** `false`

Whether to inject importmap for generated chunks. This importmap is used to optimize caching efficiency.

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 can set experimental without changing the name. 👍

## build.chunkImportMap

- **Type:** `boolean`
- **Default:** `false`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- **Default:** `false`
- **Default:** `false`
- **Experimental**

#16552 (comment)

@@ -519,6 +566,16 @@ function viteLegacyPlugin(options: Options = {}): Plugin[] {
}
}

Copy link
Member

Choose a reason for hiding this comment

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

It seems that __vite_legacy_guard inside modernChunkLegacyGuard is not being executed from anywhere.
Could you explain how it works?

The current modernChunkLegacyGuard uses the fact that browsers errors without executing a script if the script has invalid syntax (unsupported syntax). But in this case, we are judging with runtime code instead of a syntax. So I was suggesting adding the code in modernChunkLegacyGuard (i.e. changing modernChunkLegacyGuard to `if(HTMLScriptElement&&HTMLScriptElement.supports("importmap"))throw new Error;`);export function __vite_legacy_guard(){${detectModernBrowserDetector}};`.

That said, I noticed that the script that has the throw new Error won't run but still the scripts imported by that script would run. Hmm...

config.root,
normalizePath(filename),
)
const assetsBase = getBaseInHTML(relativeUrlPath, config)
Copy link
Member

Choose a reason for hiding this comment

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

I guess there aren't any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
Status: Has plan
Development

Successfully merging this pull request may close these issues.

[build] importing from hashed chunks makes caching terribly ineffective
9 participants