-
Notifications
You must be signed in to change notification settings - Fork 392
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): avoid reusing vnode.elm unless it is style tag for native #1365
Conversation
Benchmark resultsBase commit: lwc-engine-benchmark
|
Co-Authored-By: Iurie <[email protected]>
Benchmark resultsBase commit: lwc-engine-benchmark
|
I don't follow this PR... |
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. Needs tests.
Benchmark resultsBase commit: lwc-engine-benchmark
|
packages/integration-karma/test/rendering/slot-dont-recycle-vm/x/child/child.js
Outdated
Show resolved
Hide resolved
packages/integration-karma/test/rendering/slot-dont-recycle-vm/index.spec.js
Outdated
Show resolved
Hide resolved
packages/integration-karma/test/rendering/slot-dont-recycle-vm/index.spec.js
Outdated
Show resolved
Hide resolved
packages/integration-karma/test/rendering/slot-dont-recycle-vm/index.spec.js
Outdated
Show resolved
Hide resolved
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.
Second pass!
packages/integration-karma/test/rendering/elements-are-not-recycled/x/child/child.html
Outdated
Show resolved
Hide resolved
.then(() => { | ||
child.open = true; | ||
|
||
return Promise.resolve(); |
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.
We don't need to return anything here, do we?
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 thought we did (at least in jest) but we don't ... removed them
packages/integration-karma/test/rendering/elements-are-not-recycled/index.spec.js
Outdated
Show resolved
Hide resolved
packages/integration-karma/test/rendering/elements-are-not-recycled/index.spec.js
Outdated
Show resolved
Hide resolved
packages/integration-karma/test/rendering/elements-are-not-recycled/index.spec.js
Outdated
Show resolved
Hide resolved
import { createElement } from 'lwc'; | ||
import Container from 'x/container'; | ||
|
||
it('should not have have an error re-rendering slotted content', function() { |
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.
Where do we expect to see the error if elements were recycled? We should use a expect().not.toThrow()
there.
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.
Verified that the tests fail when expected to, without the bug fix. Thanks!
Details
vnode.elm
unless it is an HTMLStyleElement for native shadowDoes this PR introduce breaking changes?
No, it does not introduce breaking changes.