fix(engine): Fix composed: false getRootNode #170
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidturissini
commented
Mar 22, 2018
| if (!composed && !isUndefined(node[ViewModelReflection])) { | ||
| return node; // this is not quite the root (it is the host), but for us is sufficient | ||
| if (!composed) { | ||
| return findShadowRoot(node.parentNode); // this is not quite the root (it is the host), but for us is sufficient |
There was a problem hiding this comment.
I think this is correct, but honestly am not 100% sure
There was a problem hiding this comment.
I don't understand how is this code different. I was avoiding the usage of recursive methods, instead use the while statement to do the exact same that the new recursive method is doing. Or at least that was the intent. Let's chat more about this tomorrow morning.
There was a problem hiding this comment.
while loop over recursion
Benchmark comparisonBase commit:
|
caridy
reviewed
Mar 23, 2018
|
|
||
| describe('dom', () => { | ||
| describe('getRootNode composed true', () => { | ||
| it('should return correct value from child node', () => { | ||
| class MyComponent extends Element { |
There was a problem hiding this comment.
this is never used in this test.
caridy
reviewed
Mar 23, 2018
|
|
||
| const elm = createElement('x-parent', { is: Parent }); | ||
| document.body.appendChild(elm); | ||
| const child = elm.querySelector('div'); |
There was a problem hiding this comment.
don't do this here because it will be broken once we introduce shadow dom.
caridy
reviewed
Mar 23, 2018
| const elm = createElement('x-parent', { is: Parent }); | ||
| document.body.appendChild(elm); | ||
| const child = elm.querySelector('div'); | ||
| const match = getRootNode.call(child, { composed: true }); |
There was a problem hiding this comment.
don't test getRootNode directly because it eventually will be replaced with the one from the browser.
There was a problem hiding this comment.
The whole point of the test is ensuring that our getRootNode behaves correctly.
caridy
reviewed
Mar 23, 2018
| }); | ||
|
|
||
| it('should return correct value from self', () => { | ||
| class MyComponent extends Element { |
Benchmark comparisonBase commit:
|
caridy
approved these changes
Mar 23, 2018
Benchmark comparisonBase commit:
|
davidturissini
added a commit
that referenced
this pull request
Mar 26, 2018
* WIP: Fixing attrs to props in compiler (#167) * fix(compiler): Attributes on native DOM Elements * fix(compiler): Moving boolean attributes to props * fix(engine): Fix composed: false getRootNode (#170) * fix(engine): Fixing composed false in getRootNode * fix(engine): Removed recursion. Cleaned tests * fix(engine): lint * fix(engine): Small optimization * fix(engine): lint * chore(engine): Some more tests * fix(compiler): Fixing failing tests * chore(test): Adding assertion to integration test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Passing
composed: falsetogetRootNodereturned the same node passed in. This is incorrect and resulted in infinite loops in piercing.For reference, here is the expected behavior from Chrome (Web Component, not lightning web components):
Does this PR introduce a breaking change?