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-core): slotting mechanism to remove vnode.owner #2716

Closed
wants to merge 3 commits into from

Conversation

jodarove
Copy link
Contributor

@jodarove jodarove commented Feb 25, 2022

Details

The main goal of this PR is to remove the owner property of the vnode. The proposed solution changes how the slotting mechanism works to provide the correct owner while rendering.

vnode.owner is mainly used to associate an element with the proper shadow (and to set scoped styles) and is always the same as the VM being rendered, except for the slotted content, this PR mainly focuses on storing the owner only for the slot (and slotted content) and properly switching the owner when rendering.

This PR creates a new type of VSlot vnode, to be returned from the s() (template API) function, along with the corresponding mount/patch/unmount logic that takes care of rendering the slot and slotted content correctly while switching from "VM being rendered" while patching.

Missing:

  • Hydration logic: the hydration logic needs to accommodate this change (requires refactor(hydration): refactors hydration logic to only render once on mismatches #2729 ).
  • (compiled code) investigate if we can remove the flatten call on nodes with slots (if we flatten them for the light dom case).
  • run perf tests
  • LightDom: there is no test for it, but before this PR, the slotted content to a LightDom component is passed as part of the children, with this change, we need to patch those children dynamically (updateDynamicChildren), but in order to do it, we need an element at the beginning of both old and new children, that does not change between patches so it serves as an anchor (and to get the parentNode from it) for the diffing algo. An empty text (or comment) should be sufficient.

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.

@jodarove jodarove force-pushed the jodarove/remove-vnode-owner branch from 4dccfc6 to 4c970df Compare February 25, 2022 15:16
@jodarove jodarove force-pushed the jodarove/remove-vnode-owner branch from 4c970df to 397687d Compare February 25, 2022 15:26
@jodarove
Copy link
Contributor Author

@pmdartus @nolanlawson @caridy this is ready for review. if we decide to move forward with this PR, ill update the hydration logic.

@jodarove jodarove force-pushed the jodarove/remove-vnode-owner branch from a4a074d to e8a7ea7 Compare February 25, 2022 21:40
packages/@lwc/engine-core/src/framework/api.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/rendering.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/rendering.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/rendering.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/rendering.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/rendering.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/rendering.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/rendering.ts Outdated Show resolved Hide resolved
@caridy
Copy link
Contributor

caridy commented Mar 16, 2022

I haven't get my head around this change yet, but at first glance, it seems a lot more complicated now than the actual mechanism. I will like to understand this solution better, and explore maybe something a little simpler.

@jodarove jodarove force-pushed the jodarove/remove-vnode-owner branch from 70de56c to 07c8769 Compare March 16, 2022 22:08
@caridy
Copy link
Contributor

caridy commented Apr 19, 2022

@jodarove this is to be closed isn't it?

@caridy
Copy link
Contributor

caridy commented Apr 28, 2022

@jodarove ping

jodarove and others added 3 commits May 30, 2022 15:47
refactor: completely remove owner from vnode

fix: hydration

wip: not re-hydrating fine

wip: progress, just 3 failures

wip: progress, just 2 failures

wip: fix the complex tests

wip: format

wip: failing test for light dom

fix: light dom slot

refactor: reuse mountElement to render slot element

refactor: aChildren in vm does not need the owner

fix: generate empty text node as marker

refactor: review comments

wip: review suggestions

refactor: store owner on VSlot instead of aChildren

fix(template-compiler): remove flatten call in light dom
@pmdartus pmdartus force-pushed the jodarove/remove-vnode-owner branch from 33f3f61 to e63a8ef Compare May 30, 2022 14:05
@pmdartus
Copy link
Member

@caridy Today we are able to assign the owner to each VNode because all the VNodes are created eagerly. This will certainly be needed for implementing scoped slots. To implement this feature, a parent component will have to pass a "template fragment" to a child component so it can be stamped with data. Removing this owner property would certainly make things easier for the scoped slot implementation.

FYI @abdulsattar

@nolanlawson
Copy link
Collaborator

This seems ready to be closed. Please reopen as necessary.

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.

5 participants