fix: improve HTML script proxying#5279
Conversation
| // Referencing scripts by filePath allows for easier garbage collection & fewer memory leaks | ||
| export const htmlProxyMap = new WeakMap< | ||
| ResolvedConfig, | ||
| Map<string, Map<string, string>> |
There was a problem hiding this comment.
The ?html-proxy=[hash] is generated from an inline <script>’s inner contents.
I originally thought about having config -> hash -> script as the keys, but what if the injected scripts are changing rapidly? How would we know to blow away a script that’s no longer referenced? With us only having obfuscated hashes, it would be hard to tell when to discard items from memory.
So instead, I opted for an additional filePath key config -> filePath -> hash -> script, hence the nested Maps. This erases an HTML file’s proxy cache every time it’s re-transformed, so no unused script contents linger in memory.
Is this correct? (double-check me on this)
There was a problem hiding this comment.
The map structure looks good to me. We did the same here
, that was also a double map at one point but we ended up needing only a boolean per html, but it was the same idea.There was a problem hiding this comment.
Update for those reading: we reverted from hash to use the old index-based count. But the map structure still has filePath as it’s needed to separate inline scripts by file.
| }, | ||
|
|
||
| buildStart() { | ||
| htmlProxyMap.set(config, new Map()) |
There was a problem hiding this comment.
Cache rebuilts on buildStart() as requested
| if (result) { | ||
| return result | ||
| } | ||
| throw new Error(`No matching HTML proxy module found from ${id}`) |
There was a problem hiding this comment.
Same error is thrown as before if we can’t find a proxied script in memory.
1c326e4 to
d9d7a08
Compare
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>SSR HTML</title> |
There was a problem hiding this comment.
This playground/ssr-html example was added because I couldn’t find a way to replicate the bug using other existing setups. In the existing playground/html example I wasn’t able to modify the HTML via a Vite plugin because it seems that .html files are claimed before any Vite plugins are loaded.
I think this is worth adding tests for, because this bug still exists in .html files as well as any other compile-to-html language like .astro, .njk, .hbs, etc. But feedback welcome on if there’s a better place for these to live!
| shouldRemove = true | ||
| } else if (node.children.length) { | ||
| const contents = node.children | ||
| .map((child: any) => child?.content || '') |
There was a problem hiding this comment.
A typedef should not be needed in a lambda expression 🤔
This is mostly an indicator that something is wrongly or to less typed
In this case node.children
There was a problem hiding this comment.
In the code above we confirm this is definitely a script tag (L223) and this definitely has children (L242). But the Vue compiler flags all node.children as TemplateChildNode[]
TemplateChildNode = ElementNode | InterpolationNode | CompoundExpressionNode | TextNode | CommentNode | IfNode | IfBranchNode | ForNode | TextCallNode;
We have a string fallback in case something unexpected is encountered, so the compiled code is good IMO. But if I try and assert something other than any I get a message along the lines of .content not being found on all other possible element types therefore the assertion is incompatible.
But all that said I’m not a fan of any either and I think it should be avoided. Thoughts on replacing? I tried child: TextNode but TypeScript didn’t like that either.
There was a problem hiding this comment.
For now let's approve this PR, it's non-blocking.
Maybe I will look into such parts in the code later when I have more time again 🙂
|
About failing tests, is this working locally? Looks like the jest timeout is being hit. It is defined here Line 9 in 6f696be |
0aee619 to
84320d5
Compare
Locally the new tests were passing, but I forgot 2 things: one (trivial), the asset test needed to be updated with the hashes. But since walking that back they’re fine now. But the bigger thing is the Vue production build tests. Currently, production builds bypass the HTML transformation it seems, so I think we need to add the “read from disk“ behavior as a fallback, at least for safety? Will comment on the implementation. |
84320d5 to
d9791a2
Compare
d9791a2 to
7996c96
Compare
|
For reference, we already discussed #5061 with the team. This should count as a fix to me. |
| if (!htmlProxyMap.get(config)) { | ||
| htmlProxyMap.set(config, new Map()) | ||
| } | ||
| if (!htmlProxyMap.get(config)!.get(filePath)) { | ||
| htmlProxyMap.get(config)!.set(filePath, []) | ||
| } | ||
| htmlProxyMap.get(config)!.get(filePath)![index] = code |
There was a problem hiding this comment.
Urg I wish JS would have something like putIfAbsent
I will look if there is a TC39 proposal for that 😅
There was a problem hiding this comment.
There it is 🎉
https://github.com/tc39/proposal-upsert
Description
Resolves #5061. A quick summary is that when Vite proxies inline
<script>tags, it relies on the context of HTML files on disk. This means that if users are using a Vite or Rollup plugin that inserts any inline<script>tags, Vite will throw an error trying to find the contents from file.Without changing the URL schema for
?html-proxy, this PR saves the contents of inline<script>tags in memory at the same time they are transformed to HTML proxy URLs (rather than discarding that info, and hoping it still exists on-disk). Vite will then use load scripts from memory without needing to keep re-reading from disk again when contents may have changed.Note: the initial version of this PR did change the URL schema for
?html-proxy, but we’re attempting to skip that for now.Additional context
(see comments)
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).