[lit-labs/ssr] Shared and de-duplicated declarative styles utility #4378
Conversation
|
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultsthis-change
render
update
update-reflect
this-change, tip-of-tree, previous-release
render
update
nop-update
this-change, tip-of-tree, previous-release
render
update
this-change, tip-of-tree, previous-release
render
update
update-reflect
|
|
The size of lit-html.js and lit-core.min.js are as expected. |
| </script>`; | ||
| } | ||
|
|
||
| getStyleHash(styles: CSSResultOrNative[]): string { |
There was a problem hiding this comment.
I don't think you need a hash, unless we're trying to make this work over multiple requests. I think you should be able to use stylesheet identity on the server and a simple counter for the id.
|
The primary reason for making this prototype was to measure if there would be any benefit. I've now measured against a page in material-web.dev which uses Lit SSR. Method: I
Conclusion: Brotli is pretty fantastic and handling duplicated text content, and thus de-duping styles provides less benefit than I initially expected. The gzip benefit of 30% is compelling but I'd like feedback to see if this benefit is worth pursuing. |
|
67% smaller is a lot less to parse, regardless of compression. And it's a lot less for any downstream processing: the server, browser, any transforms or middleware. So we should at least measure first paint, but even if we didn't detect a huge improvement there I think this should still be an option because we can make it semantically correct (with some client-side changes). |
|
IMO, the main reason to add this feature is to make the styling behave as authored, using adopted stylesheets. The performance testing is mostly important to ensure it doesn't regress performance, but if it makes FCP faster, that's great. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
AndrewJakubowicz
changed the title
It makes more sense to do this separately.
|
@sorvell I've addressed your great feedback. This change still needs docs, however I'd love a re-review now that the logic has been fixed and works semantically correctly. Thank you! |
| * for a page, and use `emitCustomElementDeclarationOnce` to insert the script | ||
| * tag helper before any styles are encountered. | ||
| */ | ||
| export class DeclarativeStyleDedupeUtility { |
There was a problem hiding this comment.
Nit: "Dedupe utility" kinda sounds somehow less professional to me (I can't say why). How about "DeclarativeStyleModuleShim"? Feel free to ignore.
| </script>`; | ||
| } | ||
|
|
||
| private getStyleHash(style: CSSResultOrNative): number { |
There was a problem hiding this comment.
This isn't really a hash anymore. getStyleId maybe?
|
|
||
| *renderDedupedStyles(styles: CSSResultOrNative[]): RenderResult { | ||
| for (const style of styles) { | ||
| const styleHashId = this.getStyleHash(style); |
| * there should be only one utility instantiated which must be shared between multiple SSR | ||
| * renders. | ||
| */ | ||
| dedupeStyles?: DeclarativeStyleDedupeUtility; |
There was a problem hiding this comment.
I'm trying to think of the benefit of the user supplying an instance here, versus just making this option boolean and SSR just using a global instance. Maybe it's problematic if we're rendering across VM modules where we'd need to provide the same instance.
| yield '<style>'; | ||
| for (const style of styles) { | ||
| yield (style as CSSResult).cssText; | ||
| if (renderInfo.dedupeStyles) { |
There was a problem hiding this comment.
Should this be an instanceof check to make sure we have the right thing? I don't think we validate the option anywhere in case user isn't using TypeScript and passes in something wrong.
| // Script tags do not execute when used in innerHTML. So instead | ||
| // re-attach them to the DOM so they can execute. This must be done | ||
| // here, so it's early enough for the test to be able to assert DOM. | ||
| // These tests must use `expectMutationsOnFirstRender: true`. |
There was a problem hiding this comment.
Let's add that the script tags we care about executing are mainly the declarative style module for future readers. It is only for tests, but I assume we don't have any test that assert that script tags here won't run, right? It's meant to be inserting raw HTML that the server sends so should be fine.
| }, | ||
| ], | ||
| expectMutationsOnFirstRender: true, // The test setup manually attaches the de-dupe script tag. | ||
| stableSelectors: ['dedupe-constructable-stylesheet'], |
There was a problem hiding this comment.
should this be 'dd-constructable-stylesheet'?
| </style><lit-ssr-style-dedupe style-id="3" style="display:none;"></lit-ssr-style-dedupe><!--lit-part--><!--/lit-part--></template></test-static-styles-array><test-static-styles-array><template shadowroot="open" shadowrootmode="open"><lit-ssr-style-dedupe style-id="1" style="display:none;"></lit-ssr-style-dedupe><lit-ssr-style-dedupe style-id="3" style="display:none;"></lit-ssr-style-dedupe><!--lit-part--><!--/lit-part--></template></test-static-styles-array><!--/lit-part-->` | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Could we test that sharing the same instance of the dedupe utility allows deduping across multiple render calls?
|
hi, i am very interested in this function. is there a plan when this will be released? |
|
@AndrewJakubowicz, can you provide any updates on this? |
|
It would be great to be able to opt in to this feature. We use a security package that parses the HTML and adds nonces to any script/links etc. And right now, the HTML size is really a problem there. Would be amazing to have no repeated styles. |
|
Hi @AndrewJakubowicz , I hope this message finds you well. We are currently working on a project that heavily relies on the Lit framework, and we have encountered a challenge that we believe could be addressed by this pull request. Context and Need: It would be great if we can use this feature in a near future. Kind Regards Marcel |
|
@marcmalerei do you comrpess the HTML? gzip/brotli should compress these duplicate styles really well and significantly reduce the performance overhead. |
@prashantpalikhe, yes we do but if you have hundreds of the same component it still leads to a bloat of html. is there any progress here? |
Issue: #4357
Background
Currently, there is no way to deduplicate styles in the declarative shadow DOM. This PR provides an opt-in mechanism (if JavaScript is enabled), to deduplicates declarative shadow DOM styles from LitElement's static
stylesarray.Design
During SSR, if this feature is enabled, each
csstag present in the static styles array will be servers side rendered into a<style>element once. This element will be wrapped with the deduplicate element.After the first instance of the styles have been encountered, any subsequent instances encountered during SSR rendering will only send down the deduplication element with an id attribute which references the first instance.
During parse time, the deduplication element can synchronously pull in the styles and attach them. This avoids any FOUC.
I've made it so Constructable style sheets are used if available, falling back to cloning the style element directly.
Semantic benefits
In the case where the browser supports constructable style sheets, this utility provides semantics closer to what we would like SSR to exhibit. Shared styles will use the same underlying style sheet instance.
Thus this utility also exhibits better SSR styles semantics.
If the browser doesn't support adopting constructable style sheets, the fallback behavior is to insert a style element.
Benchmarks
tl;dr: Performance impact is unclear. The most obvious win is in
gzipanduncompressednetwork size.This is a hard feature to measure, because it impacts network size, but also will result in changes in runtime characteristics. E.g., less style tokens are parsed, but a style element or constructable style sheet is added instead.
Below are measurements I took using
material-web.devas an example of a Lit SSR'd website.What is the impact on browser performance timings. Note this was done locally using Chrome developer tools, and I jotted down the rough numbers after a couple refreshes. This is for ballpark figures.
The increased parse HTML time makes sense with deduplicated styles because we are applying the styles during parse time via the new element. This seems to slightly increase all other timings.
Risks
Doesn't work in no JavaScript environments, this is a userland solution to the following proposal: WICG/webcomponents#939