Skip to content
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

refactor(engine): optimize computation of transitive shadow mode #2803

Merged

Conversation

caridy
Copy link
Contributor

@caridy caridy commented Apr 25, 2022

Details

to avoid mixing native and synthetic shadows, a recursion was used to find the nearest owner with a shadow attached. This PR simplifies that process by doing some extra bookkeeping on the value of shadow mode, so no recursion is needed.

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

shadowMode: ShadowMode;
/** Transitive support for native Shadow DOM. A component in native mode
* transitively opts all of its descendants into native. */
readonly nearestSRMode: ShadowMode | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can now be computed on the spot when constructing the vm based on the owner's values.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a comment here explaining that SR stands for Shadow Root. (Or rename the property.) Otherwise it may not be obvious to the reader.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonly nearestSRMode: ShadowMode | null;
readonly nearestShadowMode: ShadowMode | null;

@nolanlawson
Copy link
Collaborator

Looks like there are some true test failures in these tests:

click to see
Chrome 100.0.4896.60 (Windows 7) mixed-shadow-mode transitivity when root component shadowSupportMode="any" should attach a native shadow root when possible for child synthetic component FAILED
	Error: Expected false to be true.
	    at <Jasmine>
	    at assertNativeShadowRootWhenPossible (mixed-shadow-mode/transitivity/test/mixed-shadow-mode/transitivity/index.spec.js:9:56 <- mixed-shadow-mode/transitivity/index.spec.js:120:70)
	    at UserContext.<anonymous> (mixed-shadow-mode/transitivity/test/mixed-shadow-mode/transitivity/index.spec.js:38:7 <- mixed-shadow-mode/transitivity/index.spec.js:149:11)
	    at <Jasmine>
.
Chrome 100.0.4896.60 (Windows 7) mixed-shadow-mode transitivity when root component shadowSupportMode="any" should attach a native shadow root when possible for child synthetic component with shadowSupportMode="reset" FAILED
	Error: Expected false to be true.
	    at <Jasmine>
	    at assertNativeShadowRootWhenPossible (mixed-shadow-mode/transitivity/test/mixed-shadow-mode/transitivity/index.spec.js:9:56 <- mixed-shadow-mode/transitivity/index.spec.js:120:70)
	    at UserContext.<anonymous> (mixed-shadow-mode/transitivity/test/mixed-shadow-mode/transitivity/index.spec.js:43:7 <- mixed-shadow-mode/transitivity/index.spec.js:154:11)
	    at <Jasmine>
................................................................................
......................................................................
Chrome 100.0.4896.60 (Windows 7) mixed-shadow-mode transitivity when root component shadowSupportMode="any" should attach a native shadow root when possible for slotted synthetic FAILED
	Error: Expected false to be true.
	    at <Jasmine>
	    at assertNativeShadowRootWhenPossible (mixed-shadow-mode/transitivity/test/mixed-shadow-mode/transitivity/index.spec.js:9:56 <- mixed-shadow-mode/transitivity/index.spec.js:120:70)
	    at UserContext.<anonymous> (mixed-shadow-mode/transitivity/test/mixed-shadow-mode/transitivity/index.spec.js:48:7 <- mixed-shadow-mode/transitivity/index.spec.js:159:11)
	    at <Jasmine>

Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM overall. I imagine perf-wise we are paying a small tax to add an extra prop to every VM, but on the plus side we are not crawling up a potentially long chain of Light DOM components to compute the shadowRoot for each one. That's O(n^2), so it can be pretty bad.

shadowMode: ShadowMode;
/** Transitive support for native Shadow DOM. A component in native mode
* transitively opts all of its descendants into native. */
readonly nearestSRMode: ShadowMode | null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonly nearestSRMode: ShadowMode | null;
readonly nearestShadowMode: ShadowMode | null;

@ekashida
Copy link
Member

I would also rename the title of this PR since the existing code wasn't recursive. Maybe something like optimize computation of transitive shadow mode?

@caridy caridy changed the title refactor(engine): avoid recursion when computing shadow dom mode refactor(engine): optimize computation of transitive shadow mode Apr 26, 2022
caridy and others added 4 commits April 29, 2022 11:32
Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>
Co-authored-by: Nolan Lawson <nolan@nolanlawson.com>
@ravijayaramappa ravijayaramappa force-pushed the caridy/remove-recursion-for-shadow-mode-computation branch from 1767902 to 23f4fd4 Compare April 29, 2022 18:33
@ravijayaramappa
Copy link
Contributor

Rebased with master to resolve the nucleus failure.

@caridy
Copy link
Contributor Author

caridy commented Apr 29, 2022

@ravijayaramappa are we good to go now?

@ravijayaramappa
Copy link
Contributor

🚢

@ekashida ekashida merged commit 95ce4c3 into master May 2, 2022
@ekashida ekashida deleted the caridy/remove-recursion-for-shadow-mode-computation branch May 2, 2022 21:08
nolanlawson added a commit that referenced this pull request Jun 1, 2022
nolanlawson added a commit that referenced this pull request Jun 3, 2022
…ode" (#2859)

* fix(engine-core): correctly compute shadowMode

* fix: fix comment

* fix: update packages/integration-karma/test/mixed-shadow-mode/dual-component/index.spec.js

Co-authored-by: Eugene Kashida <ekashida@gmail.com>

* fix: fix another one

* fix: improve tests

* fix: improve tests

* fix: improve test

* fix: revert

* Revert "refactor(engine): optimize computation of transitive shadow mode (#2803)"

This reverts commit 95ce4c3.

Co-authored-by: Eugene Kashida <ekashida@gmail.com>
divmain added a commit that referenced this pull request Jun 3, 2022
* 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>

* test(integration-karma): update ACT components (#2862)

* fix(engine-core): revert "optimize computation of transitive shadow mode" (#2859)

* fix(engine-core): correctly compute shadowMode

* fix: fix comment

* fix: update packages/integration-karma/test/mixed-shadow-mode/dual-component/index.spec.js

Co-authored-by: Eugene Kashida <ekashida@gmail.com>

* fix: fix another one

* fix: improve tests

* fix: improve tests

* fix: improve test

* fix: revert

* Revert "refactor(engine): optimize computation of transitive shadow mode (#2803)"

This reverts commit 95ce4c3.

Co-authored-by: Eugene Kashida <ekashida@gmail.com>

* chore: release v2.14.1 (#2866)

Co-authored-by: Nolan Lawson <nolan@nolanlawson.com>
Co-authored-by: Mohan Raj Rajamanickam <1509984+mohanraj-r@users.noreply.github.com>
Co-authored-by: Eugene Kashida <ekashida@gmail.com>
Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>
caridy pushed a commit that referenced this pull request Jun 7, 2022
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 pushed a commit that referenced this pull request Jun 7, 2022
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
ravijayaramappa added a commit that referenced this pull request Jun 13, 2022
* 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>

* test(integration-karma): update ACT components (#2862)

* fix(engine-core): revert "optimize computation of transitive shadow mode" (#2859)

* fix(engine-core): correctly compute shadowMode

* fix: fix comment

* fix: update packages/integration-karma/test/mixed-shadow-mode/dual-component/index.spec.js

Co-authored-by: Eugene Kashida <ekashida@gmail.com>

* fix: fix another one

* fix: improve tests

* fix: improve tests

* fix: improve test

* fix: revert

* Revert "refactor(engine): optimize computation of transitive shadow mode (#2803)"

This reverts commit 95ce4c3.

Co-authored-by: Eugene Kashida <ekashida@gmail.com>

* chore: release v2.14.1 (#2866)

* refactor(engine-core): encapsulate renderer as an object and allow it to be injectable in vnodes (#2763)

* refactor(engine-core): passing the renderer from an import statement in compiled templates

Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>
Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com>
Co-authored-by: Nolan Lawson <nolan@nolanlawson.com>

* test(integration-karma): more shadow transitivity tests (#2864)

* chore: release v.2.14.2 @W-7258582 (#2871)

* Revert "chore: release v2.14.1 (#2866) @W-7258582 (#2867)"

This reverts commit 518ab2e.

Co-authored-by: Nolan Lawson <nolan@nolanlawson.com>
Co-authored-by: Mohan Raj Rajamanickam <1509984+mohanraj-r@users.noreply.github.com>
Co-authored-by: Eugene Kashida <ekashida@gmail.com>
Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>
Co-authored-by: Caridy Patiño <caridy@gmail.com>
Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants