fix(custom-element): component styles are not merged with 2nd argument styles #13792
Conversation
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant App as Application
participant API as defineCustomElement
participant CE as VueElement
participant SR as ShadowRoot
Dev->>App: import defineCustomElement
App->>API: defineCustomElement(def, { extraStyles })
API->>CE: create class storing def + extraStyles
App->>CE: attach element
CE->>SR: attach shadowRoot
CE->>CE: compute fullStyles = styles + extraStyles
loop append styles
CE->>SR: append <style> (component or extra)
end
sequenceDiagram
autonumber
participant HMR as HMR Runtime
participant CE as VueElement
participant SR as ShadowRoot
HMR-->>CE: update component styles (replace)
CE->>SR: replace component style blocks
CE->>SR: preserve appended extraStyles blocks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/runtime-dom/src/apiCustomElement.ts (1)
53-59: API addition looks good; please clarify behavior in docs.Adding
extraStyles?: string[]toCustomElementOptionsis a sensible non-breaking way to merge author-supplied CSS with component-emitted styles. Please add a short JSDoc explaining:
- Merge order relative to component
styles(extraStyles appended after the component’s ownstyleson initial mount).- That
extraStylesare applied only whenshadowRoot !== false.- HMR behavior (component
stylesare replaced on reload;extraStylespersist).This will help avoid confusion with the existing
stylesfield on component defs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/runtime-dom/__tests__/customElement.spec.ts(2 hunks)packages/runtime-dom/src/apiCustomElement.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-dom/__tests__/customElement.spec.ts (1)
packages/runtime-dom/src/apiCustomElement.ts (2)
defineCustomElement(168-186)VueElement(203-699)
🪛 ast-grep (0.38.6)
packages/runtime-dom/__tests__/customElement.spec.ts
[warning] 858-858: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = <my-el-with-styles></my-el-with-styles>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 858-858: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: container.innerHTML = <my-el-with-styles></my-el-with-styles>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (3)
packages/runtime-dom/src/apiCustomElement.ts (2)
224-227: Snapshotting extraStyles across async resolution is correct.Initializing a private
_extraStylesfromthis._def.extraStylesensures the extra styles persist when the async wrapper resolves andthis._defis replaced with the inner component. This mirrors the existing_noncepattern and avoids losing extra options during async resolution. LGTM.
396-404: Dev warning now covers extraStyles — good catch.Extending the warning to trigger when
styles || this._extraStyleswhileshadowRoot: falseprevents silent failures when users rely solely onextraStyles. Looks correct.packages/runtime-dom/__tests__/customElement.spec.ts (1)
855-857: Test covers new API surface.Passing
extraStylesvia the second argument todefineCustomElementand asserting presence in shadow DOM verifies the intended merge path. Nice.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/runtime-dom/src/apiCustomElement.ts (1)
594-603: Fix validated: inject extraStyles only for the host; prevents duplicates and child HMR removalThe new fullStyles construction and the !owner guard correctly ensure:
- Host receives component styles + extraStyles once.
- Child injections bring only their own styles.
- Child HMR no longer wipes host extra CSS.
This addresses the previously raised duplication/removal issue.
Also applies to: 612-616
packages/runtime-dom/__tests__/customElement.spec.ts (1)
877-916: Added guard test: no duplication via child injection; persists across child HMR — LGTMThis directly validates the earlier bugfix: extraStyles appears once, isn’t re-injected by children, and isn’t removed on child HMR. Perfect.
🧹 Nitpick comments (2)
packages/runtime-dom/src/apiCustomElement.ts (1)
53-59: New extraStyles option: please deprecate second-arg styles and guide migrationGreat addition. To avoid future confusion where second-argument styles overwrite component-compiled styles, explicitly deprecate the CustomElementOptions.styles field and steer users to extraStyles. This is a doc-only change plus a DEV warning to help migration.
Apply this small annotation so editors/TS hint the preferred option:
export interface CustomElementOptions { - styles?: string[] + /** + * @deprecated Use `extraStyles` to avoid overriding component-compiled styles. + */ + styles?: string[] extraStyles?: string[] shadowRoot?: boolean nonce?: string configureApp?: (app: App) => void }Additionally, consider a DEV-time warning in defineCustomElement when extraOptions.styles is used so users get a clear message at runtime. (Shown here for reference; this change is outside the current hunk.)
// Inside defineCustomElement(...) just after computing Comp if (__DEV__ && extraOptions && (extraOptions as any).styles) { warn( 'defineCustomElement second-argument "styles" overrides component styles. ' + 'Use "extraStyles" to append without replacing.', ) }If you want, I can follow up with a PR comment updating the docs page to reflect this deprecation and the new guidance.
packages/runtime-dom/__tests__/customElement.spec.ts (1)
838-846: Optional: add tests for (1) DEV warning with shadowRoot: false + extraStyles and (2) nonce persistence through HMRNice coverage in styles suite. Two quick additions would lock in behavior:
- When shadowRoot: false and extraStyles provided, assert the DEV warning fires (you already warn for styles or extraStyles).
- After HMR on a host using nonce, assert all injected style tags (including extraStyles) retain the nonce.
I can add these as separate tests if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/runtime-dom/__tests__/customElement.spec.ts(2 hunks)packages/runtime-dom/src/apiCustomElement.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-dom/__tests__/customElement.spec.ts (1)
packages/runtime-dom/src/apiCustomElement.ts (2)
defineCustomElement(168-186)VueElement(203-704)
🪛 ast-grep (0.38.6)
packages/runtime-dom/__tests__/customElement.spec.ts
[warning] 858-858: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = <my-el-with-styles></my-el-with-styles>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 858-858: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: container.innerHTML = <my-el-with-styles></my-el-with-styles>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 895-895: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = <my-el-extra-child></my-el-extra-child>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 895-895: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: container.innerHTML = <my-el-extra-child></my-el-extra-child>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🔇 Additional comments (4)
packages/runtime-dom/src/apiCustomElement.ts (2)
399-404: DEV warning now considers extraStyles — LGTMIncluding this._extraStyles in the shadowRoot: false warning tightens correctness and developer feedback. No action needed.
224-227: No action needed: TS configuration guarantees safe field-initializer orderWith
"useDefineForClassFields": falseand a"target"of"es2016"in your tsconfig, TypeScript down-levels class fields into assignments in the constructor. Parameter properties (likethis._def) are assigned immediately aftersuper()and before any of those generated field assignments. Therefore, using_extraStyles?: string[] = this._def.extraStylesis safe under the current configuration, and no refactor is required.
packages/runtime-dom/__tests__/customElement.spec.ts (2)
855-862: Tests adopt extraStyles and assert merged ordering — LGTMSwitching to defineCustomElement(def, { extraStyles }) and expecting both blocks verifies the new API and merge behavior. Assertions match the prepend order.
870-875: HMR preserves extraStyles while replacing component styles — LGTMGood coverage of the host HMR path, ensuring extraStyles persist across reloads.
|
What if the user is using If merging is necessary, it isn’t mandatory to change and merge two objects without using extend.
|
|
In the documentation, the description of In the current PR, So I think the documentation might need some additional clarification? Something like:
That said, afte discussion, it feels a bit like this has turned into a feature... whereas my original intention was just to fix a bug... |
|
Also, most users probably don’t need to override styles through the second argument, just like you wouldn’t expect to provide a parameter for overriding styles in If there is such a need, it can be achieved in the following way: defineCustomElement({
...App,
styles: ['css']
})So would it be better to mark |
This pr fixes an issue where component styles are not merged with second argument styles. It also fixes an issue where HMR causes style errors when modifying the contents of a component's
<style>tag.To avoid breaking changes, this pr adds a new
extraStylesproperty to replace the originalstylesproperty. I recommend renaming thestylesproperty in documentation toextraStyles, as thestylesproperty has existed for nearly a year.https://vuejs.org/api/custom-elements.html
Summary by CodeRabbit