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
fix(engine): [218] implementation for clone node #864
Conversation
As discussed with @davidturissini we need to also patch all the created elements internal to support the case where we invoke
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
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.
LGTM!
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
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.
Also, should we add a test dealing with slotted slots?
cloneNode(deep: boolean): Node { | ||
const clone = nativeCloneNode.call(this, false); | ||
|
||
if (isFalse(deep)) { |
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.
Seems like all the browsers don't care about strict boolean equality.
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.
I'm not sure what this means @ekashida
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.
I think I understand. Browsers really only care about truthy, not strict true
or false
. I'll update this
@ekashida Does this test work for nested slots? https://github.com/salesforce/lwc/pull/864/files#diff-bc6ae3e2abe7378f62150fcc08ab5edeR304 |
Benchmark resultsBase commit: lwc-engine-benchmark
|
</x-grand> | ||
<div></div> | ||
</x-child> | ||
</template> |
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.
I was thinking more like
<template>
<x-child>
<x-grand>
<slot>
<div>No clone</div>
</slot>
</x-grand>
</x-child>
</template>
but it almost feels like it would be testing something else at that point.
Details
Clone node implementation for custom elements
Does this PR introduce a breaking change?