fix(engine-dom): refactor stylesheet API for consistency #2827

nolanlawson · 2022-05-09T21:25:09Z

Details

Rewrite of #2797 that does not remove stylesheets when unused. Instead, this is just some small refactoring with a few observable changes:

Instead of appending <style>s to the document <head>, we use document.adoptedStyleSheets if available. This unifies our logic for both global and local styles.
Use vm.hydrated instead of the global isHydrating(). vm.hydrated is the more accurate signal of whether we have inline <style>s or not, and helps address refactor(engine): removing isHydrating global flag used across packages #2806 .

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

⚠️ Yes, it does include an observable change.

See above for observable change. This has the extremely tiny chance of breaking someone if they were relying on specific ordering of <head> styles versus adoptedStyleSheets.

GUS work item

W-11120893

nolanlawson · 2022-05-09T21:26:56Z

packages/@lwc/engine-core/src/framework/stylesheet.ts

        }
-    } else if (ssr || isHydrating()) {
+    } else if (ssr || vm.hydrated) {


One downside of this is that it will slightly bloat the bundle size for environments without hydration. In the previous version of the code, if the hydrateComponent API is unused, then this becomes false and the entire else if block is tree-shaken, including the createInlineStyleVNode function.

In practice, this is like ~5 lines of code, so I don't think it's a big deal. Also, a sufficiently smart minifier could understand that vm.hydrated is always false, so we could potentially fix it in the future using a refactor. Or we could introduce a hydratable option like Svelte.

packages/@lwc/engine-dom/src/styles.ts

nolanlawson · 2022-05-10T16:26:07Z

Looks like this may actually be a perf regression. I'll look into it.

nolanlawson · 2022-05-10T21:54:00Z

I think we should ignore the perf benchmarks. The added cost for dom-ss-styled-component-create-10k-same seems to come from the lookup on the Map<string> as opposed to adoptedStyleSheets.includes(). We could special-case this for adopted style sheets (versus style elements), but then that would probably not pay off at the global document level (because it has many more stylesheets, so it would be searching through a longer array). So we would end up with a lot of special-casing, which kind of defeats the whole purpose of this refactor.

Another optimization we could do is to use create(null) instead of Map<string,...> or Set<string>. The problem is that this is optimizing for the benchmark, where there are very few unique styles overall (so the null-proto object is monomorphic instead of megamorphic). In the real world, a heavy app will have lots of different styles, so the Map or Set is probably faster there, but the benchmark would report the opposite.

I think this might be getting into micro-optimization territory (and optimizing for the benchmark). Also the regressed cases are all create-10k-same, which will be optimized by #2781. We can revisit this later if it's still regressed at the end of the release, but overall I'm not too worried.

nolanlawson · 2022-05-11T17:49:50Z

Interesting, null-proto objects are not actually faster than Maps/Sets even in our benchmarks. Or it depends on the benchmark:

nolanlawson · 2022-05-11T17:55:09Z

Let's go ahead and do this, we can revisit at the end of the release if we need to make any more optimizations.

wip: add a static vnode type wip: needs key and patch wip: all tests passing exept a few because of simple things wip: set style tokens for fragments during template evaluation wip: set element shadow token for fragments during template evaluation wip: propagate the shadow resolver key of static fragments wip: do not gen static nodes for text or comments wip: use tagedTemplate expression to replate stylesheetToken wip: use cloneNode from renderer wip: treeWalker to work in ie11 refactor: do not strip empty attr or empty class attr fix: using incorrect key wip: trim value of textNodes and review feedback fix: hydration feat: custom static element serializer wip: remove unessesary import fix: hydration fix: snapshot tests fix: missing karma test fix: test due rebase test: add test for static content needing nodeOwner fix: escape strings in serializer refactor: remove unused apis on generated code refactor: review suggestions fix: support mixed mode wip: fix compilation snapshots fix: increase 0.5kb bundlesize for engine dom fix: flapper wip: helpers.ts review wip: codegen.ts review wip: missing items from pm review wip: review comments fix: respect preserveComments and fuse $1,2 into 3 fix: svg content with the correct namespace feat(template-compiler): add option to disable static content optimizations wip: remove invalid comment chore: bump version to v2.13.0 (#2784) chore: dependencies upgrade (#2785) test: fix Node warning about event emitters (#2789) chore: run karma and integration tests in parallel (#2792) * chore: run karma and integration tests in parallel * fix: remove log lines fix(babel-plugin-component): remove import validation (#2719) test: remove flakey IE integration test (#2796) test: update test to use lwc imports (#2794) chore: Restrict further import order (#2795) chore: bump version to v2.13.1 (#2804) refactor(engine): moving vm references from dom into core (#2801) * refactor(engine): moving vm references from dom into core chore(nucleus): remove salesforcedevs/developer-website (#2807) test(integration-karma): small quality-of-life improvements (#2809) chore(deps): bump ejs from 3.1.6 to 3.1.7 (#2810) chore: weekly dependencies upgrade (#2816) * chore: weekly dependencies upgrade * fix: update yarn.lock` refactor(engine): optimize computation of transitive shadow mode (#2803) chore(deps): bump async from 2.6.3 to 2.6.4 (#2815) Bumps [async](https://github.com/caolan/async) from 2.6.3 to 2.6.4. - [Release notes](https://github.com/caolan/async/releases) - [Changelog](https://github.com/caolan/async/blob/v2.6.4/CHANGELOG.md) - [Commits](caolan/async@v2.6.3...v2.6.4) --- updated-dependencies: - dependency-name: async dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore: bump version to v2.13.2 (#2819) chore: retry failed Circle CI tests (#2814) * chore: retry failed Circle CI tests W-10946477 * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: deliberately fail a test to see what happens * chore: improve retry script * fix: whitespace * Revert "chore: deliberately fail a test to see what happens" This reverts commit 611fc34. * chore: rename to retry_karma fix(engine-core): add shim for old stylesheetTokens internal API (#2821) W-11093934 chore: bump version to v2.13.3 (#2823) fix(build): remove swc, switch back to babel and terser (#2818) feat: add freezeTemplate() API, warn on mutation (#2825) * feat: add freezeTemplate() API, warn on mutation * fix: warn on slots/renderMode as well * fix: add comment * fix: remove duplicate process.env.NODE_ENV check fix(engine-dom): refactor stylesheet API for consistency (#2827) * fix(engine-dom): refactor stylesheet API for consistency * fix: remove useless code comment * test: remove unnecessary test * test: remove unnecessary test * refactor: slight refactor * fix: add code comments * fix: add code comments * fix: add better comment fix: relax static id validation in iterations (#2830) fix(rollup-plugin): emit warnings during compilation (#2833) * fix(rollup-plugin): emit warnings during compilation Fixes #2771 W-10930894 * fix: add code comment fix(engine-dom): make feature flags work (#2812) * fix(engine-dom): make feature flags work Fixes #2811 * fix: license headers * test: fix jest tests * test: fix test * test: fix test * fix: use Eugene's technique instead * Revert "fix: use Eugene's technique instead" This reverts commit 72afdc0. * fix: use Eugene's technique instead * fix: revert unnecessary test change * fix: revert, use the elaborate test instead * fix: fix feature flags in engine-server as well perf(engine-dom): refactor style cache to reduce lookups (#2832) * perf(engine-dom): refactor style cache to reduce lookups * fix: tidy up comments * fix: update packages/@lwc/engine-dom/src/styles.ts Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com> * fix: remove semi * fix: remove "used" flag * fix: refactor * fix: refactor * fix: bring back "used" flag * fix: typo Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com> chore: update deps (#2838) test: run feature flag test code only in karma (#2835) fix: trigger slotchange event on removing slot (#2840) test(integration-karma): silence lwc rollup plugin warnings (#2836) * test(integration-karma): silence lwc rollup plugin warnings * fix: use warn API v2.11.7 (#2842) chore: release v2.14.0 (#2846) fix: only remove slot children in synthetic shadow (#2843) * fix: only remove slot children in synthetic shadow * fix: use case block fix: only add version mismatch test code in karma (#2852) test(integration-karma): ensure constructable stylesheets are re-used (#2844) * test(integration-karma): ensure constructable stylesheets are re-used * test: add test for shared style chore(nucleus): remove more downstreams (#2855) chore(nucleus): remove another downstream (#2857) docs: fix typo in template compiler readme (#2848) * docs: fix typo in template compiler readme * docs: rewording usage of lwc dynamic directive Co-authored-by: Eugene Kashida <ekashida@gmail.com> Co-authored-by: Eugene Kashida <ekashida@gmail.com> chore: fix lint test: refactor test, remove test covered in #2859 test: on second thought, bring test back

nolanlawson commented May 9, 2022

View reviewed changes

nolanlawson mentioned this pull request May 9, 2022

fix(engine-dom): remove stylesheets when unused #2797

Closed

pmdartus approved these changes May 10, 2022

View reviewed changes

packages/@lwc/engine-dom/src/styles.ts Outdated Show resolved Hide resolved

packages/@lwc/engine-dom/src/styles.ts Outdated Show resolved Hide resolved

packages/@lwc/engine-dom/src/styles.ts Show resolved Hide resolved

nolanlawson added 7 commits May 10, 2022 10:33

fix(engine-dom): refactor stylesheet API for consistency

f624b08

fix: remove useless code comment

5b96076

test: remove unnecessary test

09a56ee

test: remove unnecessary test

1cad8bb

refactor: slight refactor

6f69244

fix: add code comments

c127a0c

fix: add code comments

3c63e5f

fix: add better comment

165663a

nolanlawson force-pushed the nolan/refactor-stylesheets branch from 1ac8d87 to 165663a Compare May 10, 2022 21:56

pmdartus approved these changes May 11, 2022

View reviewed changes

nolanlawson merged commit e2bc36f into master May 11, 2022

nolanlawson deleted the nolan/refactor-stylesheets branch May 11, 2022 17:55

This was referenced May 11, 2022

refactor(engine): removing isHydrating global flag used across packages #2806

Closed

perf(engine-dom): refactor style cache to reduce lookups #2832

Merged

nolanlawson mentioned this pull request Jul 6, 2022

fix(engine-dom): append global stylesheets to document.head #2923

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(engine-dom): refactor stylesheet API for consistency #2827

fix(engine-dom): refactor stylesheet API for consistency #2827

nolanlawson commented May 9, 2022

nolanlawson May 9, 2022 •

edited

Loading

nolanlawson commented May 10, 2022

nolanlawson commented May 10, 2022 •

edited

Loading

nolanlawson commented May 11, 2022

nolanlawson commented May 11, 2022

fix(engine-dom): refactor stylesheet API for consistency #2827

fix(engine-dom): refactor stylesheet API for consistency #2827

Conversation

nolanlawson commented May 9, 2022

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

nolanlawson May 9, 2022 • edited Loading

Choose a reason for hiding this comment

nolanlawson commented May 10, 2022

nolanlawson commented May 10, 2022 • edited Loading

nolanlawson commented May 11, 2022

nolanlawson commented May 11, 2022

nolanlawson May 9, 2022 •

edited

Loading

nolanlawson commented May 10, 2022 •

edited

Loading