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
[reactive-element] Adds preserveExisting
option to adoptStyles
#3060
Conversation
Adds ability to add to an existing set of styles via adoptStyles. Fixes #3010.
🦋 Changeset detectedLatest commit: 4cff378 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 |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
reactive-element-list
render
update
update-reflect
|
// If `supportsAdoptingStyleSheets` is true then we assume CSSStyleSheet is | ||
// constructable. | ||
if (supportsAdoptingStyleSheets && this._styleSheet === undefined) { | ||
(this._styleSheet = new CSSStyleSheet()).replaceSync(this.cssText); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to cause stylesheet duplication and css text re-parsing in some maybe still relatively common scenarios:
class BaseElement extends LitElement {
static get styles() { return css`...`; }
}
class SubElementA extends BaseElement {
static get styles() { return [BaseElement.styles, css`...`]; }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will however give a speedup on codebases that don't use a getter for styles
, like all TS codebases, due to skipping a hash of a potentially long string.
I wonder if we can split the difference by caching based on the TemplateStringsArray, but only when the length is 1
, so there are no interpolations to worry about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR was based on #2978 and that change is made there, so let's discuss this change there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah... I paused that one to consider what to do about getters.
Tests adopting CSSStyleSheet and tests that we cover the case of using a constructed stylesheet when polyfilling via ShadyDOM/CSS.
We've decided not to land this. See #3010 (comment). |
Fixes #3010. Adds ability to add to an existing set of styles via
adoptStyles
.Size delta:
main: 12.75 kB │ 6.49 kB │ 5.57 kB
preserve: 13.29 kB │ 6.73 kB │ 5.8 kB