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.

Sign up for GitHub

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

Jump to bottom

fix: should always have access to slotted elements #939

Merged
merged 4 commits into from
Jan 18, 2019
Merged

fix: should always have access to slotted elements #939

merged 4 commits into from
Jan 18, 2019

Conversation

ekashida
Copy link
Member

Details

Fixes #901 where having a custom element between nested slots causes access issues.

Does this PR introduce a breaking change?

  • Yes
  • No

@ekashida ekashida requested a review from caridy January 10, 2019 08:52
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 26b1baf | Target commit: 190414d

lwc-engine-benchmark

table-append-1k metric base(26b1baf) target(190414d) trend
benchmark-table/append/1k duration 148.50 (±3.55 ms) 146.70 (±3.95 ms) -1.8ms (1.2%) 👌
table-clear-1k metric base(26b1baf) target(190414d) trend
benchmark-table/clear/1k duration 5.80 (±0.40 ms) 6.00 (±0.50 ms) +0.2ms (3.4%) 👌
table-create-10k metric base(26b1baf) target(190414d) trend
benchmark-table/create/10k duration 872.30 (±6.50 ms) 884.50 (±8.05 ms) +12.2ms (1.4%) 👎
table-create-1k metric base(26b1baf) target(190414d) trend
benchmark-table/create/1k duration 118.35 (±3.15 ms) 115.75 (±2.25 ms) -2.6ms (2.2%) 👍
table-update-10th-1k metric base(26b1baf) target(190414d) trend
benchmark-table/update-10th/1k duration 74.60 (±1.55 ms) 74.60 (±1.60 ms) 0.0ms (0.0%) 👌
tablecmp-append-1k metric base(26b1baf) target(190414d) trend
benchmark-table-component/append/1k duration 235.70 (±16.30 ms) 214.55 (±8.35 ms) -21.1ms (9.0%) 👍
tablecmp-clear-1k metric base(26b1baf) target(190414d) trend
benchmark-table-component/clear/1k duration 11.75 (±1.70 ms) 11.40 (±1.45 ms) -0.3ms (3.0%) 👌
tablecmp-create-10k metric base(26b1baf) target(190414d) trend
benchmark-table-component/create/10k duration 1696.40 (±13.25 ms) 1724.45 (±23.00 ms) +28.0ms (1.7%) 👎
tablecmp-create-1k metric base(26b1baf) target(190414d) trend
benchmark-table-component/create/1k duration 215.10 (±4.50 ms) 207.00 (±5.25 ms) -8.1ms (3.8%) 👍
tablecmp-update-10th-1k metric base(26b1baf) target(190414d) trend
benchmark-table-component/update-10th/1k duration 67.05 (±5.80 ms) 68.65 (±4.60 ms) +1.6ms (2.4%) 👌
wc-append-1k metric base(26b1baf) target(190414d) trend
benchmark-table-wc/append/1k duration 253.90 (±4.55 ms) 249.55 (±5.90 ms) -4.3ms (1.7%) 👍
wc-clear-1k metric base(26b1baf) target(190414d) trend
benchmark-table-wc/clear/1k duration 21.35 (±1.90 ms) 21.55 (±2.35 ms) +0.2ms (0.9%) 👌
wc-create-10k metric base(26b1baf) target(190414d) trend
benchmark-table-wc/create/10k duration 1931.90 (±22.80 ms) 1920.40 (±33.45 ms) -11.5ms (0.6%) 👍
wc-create-1k metric base(26b1baf) target(190414d) trend
benchmark-table-wc/create/1k duration 218.30 (±3.60 ms) 220.50 (±4.55 ms) +2.2ms (1.0%) 👎
wc-update-10th-1k metric base(26b1baf) target(190414d) trend
benchmark-table-wc/update-10th/1k duration 73.05 (±5.15 ms) 69.80 (±5.40 ms) -3.3ms (4.4%) 👌

caridy added a commit that referenced this pull request Jan 12, 2019
@caridy
fix(engine): integratino tests for PR #939
c6a76b7
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 7dbeff5 | Target commit: c6a76b7

lwc-engine-benchmark

table-append-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table/append/1k duration 150.65 (±4.40 ms) 150.45 (±6.90 ms) -0.2ms (0.1%) 👌
table-clear-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table/clear/1k duration 6.10 (±0.40 ms) 5.90 (±0.30 ms) -0.2ms (3.3%) 👌
table-create-10k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table/create/10k duration 904.20 (±5.35 ms) 875.20 (±5.45 ms) -29.0ms (3.2%) 👍
table-create-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table/create/1k duration 119.10 (±3.30 ms) 117.35 (±3.55 ms) -1.8ms (1.5%) 👌
table-update-10th-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table/update-10th/1k duration 77.55 (±5.60 ms) 74.80 (±1.75 ms) -2.8ms (3.5%) 👍
tablecmp-append-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table-component/append/1k duration 251.85 (±5.40 ms) 256.60 (±5.90 ms) +4.8ms (1.9%) 👌
tablecmp-clear-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table-component/clear/1k duration 12.10 (±1.55 ms) 11.90 (±1.30 ms) -0.2ms (1.7%) 👌
tablecmp-create-10k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table-component/create/10k duration 1774.40 (±28.35 ms) 1719.75 (±16.70 ms) -54.7ms (3.1%) 👍
tablecmp-create-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table-component/create/1k duration 210.30 (±6.70 ms) 212.90 (±5.95 ms) +2.6ms (1.2%) 👌
tablecmp-update-10th-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table-component/update-10th/1k duration 69.70 (±5.45 ms) 70.65 (±6.45 ms) +1.0ms (1.4%) 👌
wc-append-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table-wc/append/1k duration 255.05 (±5.95 ms) 255.80 (±5.60 ms) +0.8ms (0.3%) 👌
wc-clear-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table-wc/clear/1k duration 22.60 (±2.35 ms) 21.60 (±2.60 ms) -1.0ms (4.4%) 👌
wc-create-10k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table-wc/create/10k duration 1964.55 (±10.00 ms) 1987.15 (±11.05 ms) +22.6ms (1.2%) 👎
wc-create-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table-wc/create/1k duration 224.05 (±5.10 ms) 225.60 (±5.05 ms) +1.6ms (0.7%) 👌
wc-update-10th-1k metric base(7dbeff5) target(c6a76b7) trend
benchmark-table-wc/update-10th/1k duration 71.80 (±5.45 ms) 73.15 (±6.05 ms) +1.4ms (1.9%) 👌

@caridy
Copy link
Contributor

caridy commented Jan 12, 2019

this is ready for review.

pmdartus
pmdartus reviewed Jan 14, 2019
View reviewed changes
packages/@lwc/engine/src/faux-shadow/traverse.ts
// that element is an slot, then the node is considered slotted
// TODO: add the examples
Copy link
Member

Choose a reason for hiding this comment

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

Missing examples.

pmdartus
pmdartus reviewed Jan 14, 2019
View reviewed changes
Copy link
Member

@pmdartus pmdartus left a comment

Choose a reason for hiding this comment

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

While I tested it locally and wasn't able to find an edge case. I have a hard time to follow enough to review the code.

caridy
caridy approved these changes Jan 17, 2019
View reviewed changes
Copy link
Contributor

@caridy caridy left a comment

Choose a reason for hiding this comment

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

I'm approving this on behalf @ekashida since he creates the tests, and I fixed the code, but he has reviewed my code.

@caridy caridy force-pushed the fix-slotted-access branch from c6a76b7 to 2551033 Compare January 17, 2019 23:44
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 4931eec | Target commit: 2551033

lwc-engine-benchmark

table-append-1k metric base(4931eec) target(2551033) trend
benchmark-table/append/1k duration 153.70 (±5.05 ms) 154.40 (±4.05 ms) +0.7ms (0.5%) 👌
table-clear-1k metric base(4931eec) target(2551033) trend
benchmark-table/clear/1k duration 6.10 (±0.45 ms) 6.30 (±0.40 ms) +0.2ms (3.3%) 👌
table-create-10k metric base(4931eec) target(2551033) trend
benchmark-table/create/10k duration 917.40 (±4.60 ms) 912.55 (±7.05 ms) -4.9ms (0.5%) 👍
table-create-1k metric base(4931eec) target(2551033) trend
benchmark-table/create/1k duration 118.85 (±3.80 ms) 120.25 (±2.95 ms) +1.4ms (1.2%) 👌
table-update-10th-1k metric base(4931eec) target(2551033) trend
benchmark-table/update-10th/1k duration 76.10 (±2.10 ms) 87.00 (±2.40 ms) +10.9ms (14.3%) 👎
tablecmp-append-1k metric base(4931eec) target(2551033) trend
benchmark-table-component/append/1k duration 253.65 (±4.95 ms) 224.30 (±16.15 ms) -29.3ms (11.6%) 👍
tablecmp-clear-1k metric base(4931eec) target(2551033) trend
benchmark-table-component/clear/1k duration 12.20 (±1.75 ms) 11.50 (±1.75 ms) -0.7ms (5.7%) 👌
tablecmp-create-10k metric base(4931eec) target(2551033) trend
benchmark-table-component/create/10k duration 1754.45 (±17.00 ms) 1791.55 (±19.45 ms) +37.1ms (2.1%) 👎
tablecmp-create-1k metric base(4931eec) target(2551033) trend
benchmark-table-component/create/1k duration 211.50 (±6.55 ms) 209.90 (±6.70 ms) -1.6ms (0.8%) 👌
tablecmp-update-10th-1k metric base(4931eec) target(2551033) trend
benchmark-table-component/update-10th/1k duration 72.75 (±5.10 ms) 73.20 (±6.85 ms) +0.5ms (0.6%) 👌
wc-append-1k metric base(4931eec) target(2551033) trend
benchmark-table-wc/append/1k duration 263.70 (±6.30 ms) 261.75 (±5.85 ms) -2.0ms (0.7%) 👌
wc-clear-1k metric base(4931eec) target(2551033) trend
benchmark-table-wc/clear/1k duration 22.10 (±1.65 ms) 22.60 (±2.00 ms) +0.5ms (2.3%) 👌
wc-create-10k metric base(4931eec) target(2551033) trend
benchmark-table-wc/create/10k duration 1983.70 (±9.25 ms) 1976.55 (±12.85 ms) -7.1ms (0.4%) 👍
wc-create-1k metric base(4931eec) target(2551033) trend
benchmark-table-wc/create/1k duration 228.90 (±5.80 ms) 226.80 (±4.40 ms) -2.1ms (0.9%) 👌
wc-update-10th-1k metric base(4931eec) target(2551033) trend
benchmark-table-wc/update-10th/1k duration 71.40 (±5.25 ms) 69.95 (±5.40 ms) -1.5ms (2.0%) 👌

@caridy caridy merged commit b767131 into master Jan 18, 2019
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 5c16e22 | Target commit: a01a20a

lwc-engine-benchmark

table-append-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table/append/1k duration 149.20 (±5.05 ms) 150.70 (±4.85 ms) +1.5ms (1.0%) 👌
table-clear-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table/clear/1k duration 5.70 (±0.40 ms) 5.80 (±0.20 ms) +0.1ms (1.8%) 👌
table-create-10k metric base(5c16e22) target(a01a20a) trend
benchmark-table/create/10k duration 865.20 (±9.75 ms) 911.85 (±6.40 ms) +46.6ms (5.4%) 👎
table-create-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table/create/1k duration 116.75 (±3.35 ms) 118.15 (±3.25 ms) +1.4ms (1.2%) 👎
table-update-10th-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table/update-10th/1k duration 85.20 (±2.25 ms) 75.80 (±1.60 ms) -9.4ms (11.0%) 👍
tablecmp-append-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table-component/append/1k duration 220.45 (±14.35 ms) 247.00 (±12.50 ms) +26.6ms (12.0%) 👎
tablecmp-clear-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table-component/clear/1k duration 12.25 (±1.95 ms) 11.90 (±1.70 ms) -0.3ms (2.9%) 👌
tablecmp-create-10k metric base(5c16e22) target(a01a20a) trend
benchmark-table-component/create/10k duration 1759.20 (±15.00 ms) 1764.20 (±22.15 ms) +5.0ms (0.3%) 👌
tablecmp-create-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table-component/create/1k duration 208.60 (±5.85 ms) 214.25 (±4.90 ms) +5.7ms (2.7%) 👎
tablecmp-update-10th-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table-component/update-10th/1k duration 69.90 (±4.85 ms) 70.10 (±5.65 ms) +0.2ms (0.3%) 👌
wc-append-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table-wc/append/1k duration 256.70 (±6.15 ms) 259.75 (±7.20 ms) +3.0ms (1.2%) 👌
wc-clear-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table-wc/clear/1k duration 21.65 (±2.15 ms) 22.15 (±2.65 ms) +0.5ms (2.3%) 👌
wc-create-10k metric base(5c16e22) target(a01a20a) trend
benchmark-table-wc/create/10k duration 1980.10 (±12.80 ms) 2000.20 (±14.25 ms) +20.1ms (1.0%) 👎
wc-create-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table-wc/create/1k duration 221.50 (±5.25 ms) 223.20 (±6.20 ms) +1.7ms (0.8%) 👌
wc-update-10th-1k metric base(5c16e22) target(a01a20a) trend
benchmark-table-wc/update-10th/1k duration 68.55 (±4.35 ms) 71.35 (±5.45 ms) +2.8ms (4.1%) 👌

caridy pushed a commit that referenced this pull request Jan 18, 2019
@ekashida @caridy
fix: should always have access to slotted elements (#939)
b1d8c84 
* fix(engine): finding slotted elements when querying
@caridy caridy mentioned this pull request Jan 18, 2019
Merged
caridy added a commit that referenced this pull request Jan 18, 2019
@caridy
fix: should always have access to slotted elements (#939) (#982)
2a72188 
* fix(engine): finding slotted elements when querying
@ekashida ekashida deleted the fix-slotted-access branch January 18, 2019 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmdartus pmdartus pmdartus left review comments

@caridy caridy caridy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ekashida @caridy @pmdartus