fix(lit): replace unsafeCSS with CSSStyleSheet for structural styles#633
fix(lit): replace unsafeCSS with CSSStyleSheet for structural styles#633iamrajhans wants to merge 3 commits intogoogle:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the security concern of using unsafeCSS by replacing it with the safer CSSStyleSheet API. The implementation is solid, with a good fallback for environments where CSSStyleSheet is not supported. I have one suggestion to improve debuggability by logging potential errors during stylesheet creation.
renderers/lit/src/0.8/ui/styles.ts
Outdated
There was a problem hiding this comment.
It's good practice to log errors that are caught, even when providing a fallback. Swallowing the error here could make it difficult to debug if new CSSStyleSheet() or replaceSync() fails for an unexpected reason. Adding a console.error would provide visibility into such issues without changing the fallback behavior.
} catch (e) {
console.error('Failed to construct structural styles:', e);
return [];
}There was a problem hiding this comment.
This is good advice, and if we were to keep this behavior of returning an empty array, we should also console.error in the cases where CSSStyleSheet() is not available. (But IMO, we shouldn't)
There was a problem hiding this comment.
I left some comments, and I think @ava-cassiopeia should have final say, but this looks good to me! Thanks for the fix!
(I'm also modifying the description of this PR to link it to the issue about removing unsafeCSS)
renderers/lit/src/0.8/ui/styles.ts
Outdated
There was a problem hiding this comment.
The CSSStyleSheet() constructor seems to be very widely supported, I don't think this check is necessary.
(In case we want to keep this, I'd suggest adding a console.error on the cases where we return [] to let the user know why their stylesheets are not working!)
renderers/lit/src/0.8/ui/styles.ts
Outdated
There was a problem hiding this comment.
In what cases would this fail? Why would we want this try-catch here? (I don't see any "Exceptions" sections in the docs?)
renderers/lit/src/0.8/ui/styles.ts
Outdated
There was a problem hiding this comment.
This is good advice, and if we were to keep this behavior of returning an empty array, we should also console.error in the cases where CSSStyleSheet() is not available. (But IMO, we shouldn't)
ava-cassiopeia
left a comment
There was a problem hiding this comment.
This generally LGTM, but I think @ditman has some solid points, so I'll wait to approve until those are addressed.
This PR removes
unsafeCSSusage from the Lit renderer to fix internal build policy failures reported in issue #462.Specifically, in
renderers/lit/src/0.8/ui/styles.ts,unsafeCSS(Styles.structuralStyles)was replaced with a safer stylesheet construction flow:CSSStyleSheetStyles.structuralStylesusingreplaceSync[]as a fallback whenCSSStyleSheetis unavailable or failsThis preserves behavior (shared structural styles still applied) while eliminating direct
unsafeCSSusage.Fixes: #462
Pre-launch Checklist