feat(html): implement asset URL rewriting for <iframe srcdoc> in HTML#21226
Conversation
🦋 Changeset detectedLatest commit: 06bb04b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21226 +/- ##
=======================================
Coverage 92.80% 92.81%
=======================================
Files 591 592 +1
Lines 64726 64785 +59
Branches 18040 18050 +10
=======================================
+ Hits 60072 60131 +59
Misses 4654 4654
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | benchmark "asset-modules-inline", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
389.5 KB | 1,230.7 KB | -68.35% |
| ❌ | Memory | benchmark "many-chunks-esm", scenario '{"name":"mode-production","mode":"production"}' |
6.7 MB | 8.7 MB | -22.62% |
| ⚡ | Memory | benchmark "wasm-modules-async", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
332.4 KB | 248.2 KB | +33.91% |
| ⚡ | Memory | benchmark "many-modules-commonjs", scenario '{"name":"mode-production","mode":"production"}' |
9.7 MB | 7.4 MB | +32.14% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing bjohansebas:feat/html-iframe-srcdoc (06bb04b) with main (ecc00c0)
|
We should not hardcode tags and attributes, it should be live in sources option by default, so other developers can apply this logic to other tags and attributes, also don’t forget, parsing srcdoc is not the same like html file by spec, there is an another logic, our parser is not fully ready for this, we need to introduce other function to parse it as I remember, verify it, also absolute URLs is already handled by output experimental option, don’t ignore them, the same for other types root relative and etc, no need to make todo because nothing to fix |
| * processed HTML read from the nested module's `html` code-generation data, | ||
| * re-escaped so the surrounding attribute stays valid. | ||
| */ | ||
| class HtmlInlineHtmlDependency extends ModuleDependency { |
There was a problem hiding this comment.
With this, it is not treated like a normal HTML document. Also, before that, the entire document is encoded to Base64
|
@bjohansebas looks good, let's rebase and we can merge |
94d63be to
23fe0b7
Compare
|
@alexander-akait done |
| rules.push({ | ||
| dependency: "html-srcdoc", | ||
| generator: { | ||
| extract: true |
There was a problem hiding this comment.
I am not sure we really need to extract here, but let's resolve it later, will investigate it in future
There was a problem hiding this comment.
What if we add support for an "inline" string so I can simplify things?
| break; | ||
| } | ||
| } | ||
| if (inlineSrcdocOnly) continue; |
There was a problem hiding this comment.
This is bad code, it will be bad for performance, any extra loops for dependencies should be used only when no one other solution
23fe0b7 to
27b5d47
Compare
|
Memoize the
|
|
With my change to extract "inline", the performance stays the same |
|
@bjohansebas sorry for extra rebase, can you do again and we can merge |
…es and fix asset URL rewriting
- Updated WebpackOptions.json to include `srcdoc` as a valid type for attribute parsing and bundling. - Modified HtmlParserOptions.check.js to validate `srcdoc` type in the HTML parser options. - Enhanced snapshot tests to cover new `srcdoc` functionality in iframe elements. - Created new test cases for handling `srcdoc` in custom tags and attributes. - Updated type definitions to reflect the addition of `srcdoc` in HtmlParserOptions and SourceType.
…es and update HTML parsing logic
…upport and additional tests
- Updated WebpackOptions schema to allow "inline" as a valid option for the extract property, alongside boolean values. - Modified HtmlGeneratorOptions validation logic to accommodate the new "inline" option. - Adjusted unit tests to reflect the change in default extraction behavior from true to "inline". - Updated TypeScript definitions to include "inline" as a valid type for the extract property.
…s and configuration
3475434 to
3447067
Compare
|
Let’s fix lint, ignore bun, they are a little but unstable, WIP on it |
1c7120f to
0e52f3c
Compare
|
This PR is packaged and the instant preview is available (164a6ff). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@164a6ff
yarn add -D webpack@https://pkg.pr.new/webpack@164a6ff
pnpm add -D webpack@https://pkg.pr.new/webpack@164a6ff |
Summary
What kind of change does this PR introduce?
Did you add tests for your changes?
Does this PR introduce a breaking change?
If relevant, what needs to be documented once your changes are merged or what have you already documented?
Use of AI