fix(engine): deep manual dom elements fix #874
Conversation
davidturissini
changed the title
| // If node aleady has an ownerkey, we can skip | ||
| // Note: checking if a node as any ownerKey is not enough | ||
| // because this element could be moved from one | ||
| // shadow to another |
There was a problem hiding this comment.
Can we replace this comment about handling the case where the node is moved from one shadow to another, with a test? :)
| setCSSToken(node, shadowToken); | ||
| const { childNodes } = node; | ||
| for (let i = 0, len = childNodes.length; i < len; i += 1) { | ||
| const node: Node = childNodes[i]; |
There was a problem hiding this comment.
I'm still a ts n00b but isn't the Node type inferred?
There was a problem hiding this comment.
I'll double check, but I believe TS thinks that childNodes[i] can return Node | undefined
| handleClick() { | ||
| const timeout = window.setTimeout(() => { | ||
| this.errorMessage = 'No error'; | ||
| }, 200); |
There was a problem hiding this comment.
Non-zero values always throw me off. Since all the operations are sync, we can use 0 here right?
There was a problem hiding this comment.
MutationObserver operations are not sync, which is where the error would come from. They are run in a micro task which should execute before any timeouts, so a 0 duration timeout would probably work but I have the extra delay just to be safe
There was a problem hiding this comment.
As you pointed out @davidturissini, by spec the MO callback need in a micro-task. I would be in favor of using a 0 value for the timeout.
| }); | ||
| browser.waitUntil(() => { | ||
| return errorElm.getText() === 'No error'; | ||
| }, 1000, 'Manually adding elements with children should not error'); |
There was a problem hiding this comment.
Can we not specify a custom timeout here so that the default value is used implicitly?
There was a problem hiding this comment.
Can you clarify this? I don't quite understand what you mean
There was a problem hiding this comment.
| }, 1000, 'Manually adding elements with children should not error'); | |
| it('should not error when manually adding elements with children', () => { | |
| ... | |
| browser.waitUntil(() => { | |
| return errorElm.getText() === 'No error'; | |
| }); | |
| // assert(true)? |
| return; | ||
| } | ||
| setNodeOwnerKey(node, ownerKey); | ||
| if (node instanceof HTMLElement) { |
There was a problem hiding this comment.
This condition needs to be updated with #872 depending on which PR lands first.
Benchmark resultsBase commit: lwc-engine-benchmark
|
Details
Fixes issue where manually appending non-empty DOM elements to
lwc:dom="manual"would error when children were modified.Does this PR introduce a breaking change?