Skip to content
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: getRootNode when composed is false and the element is the root #337

Merged
merged 2 commits into from
May 22, 2018

Conversation

pmdartus
Copy link
Member

Details

Fix error throw in getRootNode when the component is the root of the component tree.

Does this PR introduce a breaking change?

  • Yes
  • No

@pmdartus pmdartus requested a review from davidturissini May 22, 2018 18:11
* TODO: Once we start using the real shadowDOM, this method should be replaced by:
* const { getRootNode } = Node.prototype;
*/
function getRootNode(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems functional equivalent, just cosmetic changes. Can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's correct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.

* It doesn't returns actually the root but the host. This approximation is sufficiant
* in our case.
*/
function findComposedRootNode(node: Node): Node {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain what the problem was? in which scenario?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem was finding the root node on an element rendered in interop. Since there is no parent VM, it was traversing to the document root. Because this isn't inside of a closed root, going to the document root seems correct.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a test case for this. https://github.com/salesforce/lwc/pull/337/files#diff-2a67454c78a85e47d3e8214bb464d731R114

However, if you look at the diff, the composed was set to true instead of false, explaining why we didn't catch that before. I fixed the test.

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 51adcb6 | Target commit: 729538e

lwc-engine-benchmark

table-append-1k metric base(51adcb6) target(729538e) trend
benchmark-table/append/1k duration 145.00 (± 5.60 ms) 146.30 (± 5.10 ms) -0.90% 👌
table-clear-1k metric base(51adcb6) target(729538e) trend
benchmark-table/clear/1k duration 11.80 (± 0.50 ms) 11.70 (± 0.50 ms) 0.85% 👌
table-create-10k metric base(51adcb6) target(729538e) trend
benchmark-table/create/10k duration 844.30 (± 3.70 ms) 863.40 (± 6.80 ms) -2.26% 👎
table-create-1k metric base(51adcb6) target(729538e) trend
benchmark-table/create/1k duration 101.70 (± 2.00 ms) 101.50 (± 2.10 ms) 0.20% 👌
table-update-10th-1k metric base(51adcb6) target(729538e) trend
benchmark-table/update-10th/1k duration 92.50 (± 5.15 ms) 93.80 (± 4.40 ms) -1.41% 👌
tablecmp-append-1k metric base(51adcb6) target(729538e) trend
benchmark-table-component/append/1k duration 242.10 (± 5.20 ms) 247.30 (± 4.30 ms) -2.15% 👎
tablecmp-clear-1k metric base(51adcb6) target(729538e) trend
benchmark-table/clear/1k duration 33.00 (± 1.10 ms) 32.40 (± 1.40 ms) 1.82% 👌
tablecmp-create-10k metric base(51adcb6) target(729538e) trend
benchmark-table-component/create/10k duration 1712.50 (± 6.50 ms) 1668.50 (± 11.70 ms) 2.57% 👍
tablecmp-create-1k metric base(51adcb6) target(729538e) trend
benchmark-table-component/create/1k duration 188.70 (± 3.90 ms) 191.40 (± 3.80 ms) -1.43% 👌
tablecmp-update-10th-1k metric base(51adcb6) target(729538e) trend
benchmark-table-component/update-10th/1k duration 74.40 (± 5.10 ms) 73.00 (± 2.90 ms) 1.88% 👌

@pmdartus pmdartus merged commit 8dd8392 into master May 22, 2018
@pmdartus pmdartus deleted the pmdartus/fix-getFindShadowRoot branch May 22, 2018 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants