fix(engine): adding more tests for proxies #407
Conversation
| const elm = createElement('x-foo', { is: MyComponent }); | ||
| document.body.appendChild(elm); | ||
| const root = getHostShadowRoot(elm); | ||
| root.querySelector('div').innerHTML = 'something'; |
There was a problem hiding this comment.
@davidturissini @diervo in theory this should be fine since we don't have a setter trap for the proxy, which means the proxy will attempt to do the set into the original proxy target.
Since we only proxy nodes and methods of nodes, and no API that I know of has a setter that expects a node, we should be fine. Eventually we can add a set trap that unwrap the value before assigning it into the original target, but I could not find a DOM api that exemplify this potential problem.
| const root = getHostShadowRoot(elm); | ||
| const div = root.querySelector('div'); | ||
| const p = root.querySelector('p'); | ||
| expect(div.contains(p)).toBe(true); |
There was a problem hiding this comment.
this is testing that when invoking methods, we do the proper unwrap before calling the underlaying method with the right context.
Benchmark resultsBase commit: lwc-engine-benchmark
|
| return html; | ||
| } | ||
| renderedCallback() { | ||
| this.template.querySelector('span').innerHTML = '<i>something</i>'; |
There was a problem hiding this comment.
@davidturissini @diervo in theory this should be fine since we don't have a setter trap for the proxy, which means the proxy will attempt to do the set into the original proxy target.
Since we only proxy nodes and methods of nodes, and no API that I know of has a setter that expects a node, we should be fine. Eventually we can add a set trap that unwrap the value before assigning it into the original target, but I could not find a DOM api that exemplify this potential problem.
Benchmark resultsBase commit: lwc-engine-benchmark
|
| if (key === TargetSlot) { | ||
| return false; | ||
| } | ||
| originalTarget[key] = unwrap(value); |
There was a problem hiding this comment.
@davidturissini @ekashida this is needed because jsdom does support DOM elements as proxies (I suspect because it doesn't have a way to know for sure), while the browser fails on the brand check on the proxy. We need integration tests for these cases here because our units will not capture those failures.
Benchmark resultsBase commit: lwc-engine-benchmark
|
@diervo these tests are supposed to cover a lot of ground in terms of potential illegal invocations. It will be interesting to see which line triggers that problem in core.
Does this PR introduce a breaking change?