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(engine): render() can only return qualifying templates #764

Merged
merged 2 commits into from
Nov 5, 2018

Conversation

caridy
Copy link
Contributor

@caridy caridy commented Oct 24, 2018

Details

Previously, we supported render() to return undefined or qualifying template. With this PR, we will enforce that if there is a render method, it should return a qualifying template.

Does this PR introduce a breaking change?

  • Yes

@caridy caridy requested review from diervo and ekashida October 24, 2018 20:49
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 00a13cc | Target commit: 25013b0

lwc-engine-benchmark

table-append-1k metric base(00a13cc) target(25013b0) trend
benchmark-table/append/1k duration 156.25 (±4.80 ms) 150.45 (±5.20 ms) -5.8ms (3.7%) 👍
table-clear-1k metric base(00a13cc) target(25013b0) trend
benchmark-table/clear/1k duration 6.60 (±0.30 ms) 6.50 (±0.40 ms) -0.1ms (1.5%) 👌
table-create-10k metric base(00a13cc) target(25013b0) trend
benchmark-table/create/10k duration 879.85 (±5.55 ms) 877.30 (±7.25 ms) -2.5ms (0.3%) 👌
table-create-1k metric base(00a13cc) target(25013b0) trend
benchmark-table/create/1k duration 114.90 (±2.40 ms) 115.75 (±2.70 ms) +0.8ms (0.7%) 👌
table-update-10th-1k metric base(00a13cc) target(25013b0) trend
benchmark-table/update-10th/1k duration 86.05 (±3.00 ms) 79.85 (±2.60 ms) -6.2ms (7.2%) 👌
tablecmp-append-1k metric base(00a13cc) target(25013b0) trend
benchmark-table-component/append/1k duration 215.90 (±13.20 ms) 240.10 (±13.80 ms) +24.2ms (11.2%) 👎
tablecmp-clear-1k metric base(00a13cc) target(25013b0) trend
benchmark-table-component/clear/1k duration 11.40 (±1.30 ms) 11.80 (±1.50 ms) +0.4ms (3.5%) 👌
tablecmp-create-10k metric base(00a13cc) target(25013b0) trend
benchmark-table-component/create/10k duration 1656.90 (±12.90 ms) 1662.50 (±23.85 ms) +5.6ms (0.3%) 👎
tablecmp-create-1k metric base(00a13cc) target(25013b0) trend
benchmark-table-component/create/1k duration 198.90 (±4.80 ms) 200.60 (±4.10 ms) +1.7ms (0.9%) 👌
tablecmp-update-10th-1k metric base(00a13cc) target(25013b0) trend
benchmark-table-component/update-10th/1k duration 69.35 (±4.25 ms) 72.95 (±5.15 ms) +3.6ms (5.2%) 👌
wc-append-1k metric base(00a13cc) target(25013b0) trend
benchmark-table-wc/append/1k duration 231.25 (±9.40 ms) 246.40 (±14.75 ms) +15.1ms (6.6%) 👎
wc-clear-1k metric base(00a13cc) target(25013b0) trend
benchmark-table-wc/clear/1k duration 22.15 (±2.55 ms) 23.60 (±2.10 ms) +1.5ms (6.5%) 👎
wc-create-10k metric base(00a13cc) target(25013b0) trend
benchmark-table-wc/create/10k duration 1685.95 (±52.35 ms) 1942.15 (±63.10 ms) +256.2ms (15.2%) 👎
wc-create-1k metric base(00a13cc) target(25013b0) trend
benchmark-table-wc/create/1k duration 206.80 (±5.20 ms) 227.60 (±4.30 ms) +20.8ms (10.1%) 👎
wc-update-10th-1k metric base(00a13cc) target(25013b0) trend
benchmark-table-wc/update-10th/1k duration 75.05 (±6.25 ms) 78.55 (±5.95 ms) +3.5ms (4.7%) 👎

@@ -35,6 +37,7 @@ describe('patch', () => {
class MyComponent extends LightningElement {
renderedCallback() {
flag = true;
return emptyTemplate;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does renderedCallback need to return this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uff, my bad!

Copy link
Member

Choose a reason for hiding this comment

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

Removed

@diervo
Copy link
Contributor

diervo commented Oct 24, 2018

@caridy do another empty commit to double check the numbers are not as odd.

Copy link
Member

@ekashida ekashida left a comment

Choose a reason for hiding this comment

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

Changes look good but we'll need to exercise caution when releasing this.

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 00a13cc | Target commit: 4df6162

lwc-engine-benchmark

table-append-1k metric base(00a13cc) target(4df6162) trend
benchmark-table/append/1k duration 156.25 (±4.80 ms) 146.95 (±4.45 ms) -9.3ms (6.0%) 👍
table-clear-1k metric base(00a13cc) target(4df6162) trend
benchmark-table/clear/1k duration 6.60 (±0.30 ms) 5.95 (±0.30 ms) -0.6ms (9.8%) 👍
table-create-10k metric base(00a13cc) target(4df6162) trend
benchmark-table/create/10k duration 879.85 (±5.55 ms) 879.65 (±6.30 ms) -0.2ms (0.0%) 👌
table-create-1k metric base(00a13cc) target(4df6162) trend
benchmark-table/create/1k duration 114.90 (±2.40 ms) 113.55 (±3.10 ms) -1.4ms (1.2%) 👌
table-update-10th-1k metric base(00a13cc) target(4df6162) trend
benchmark-table/update-10th/1k duration 86.05 (±3.00 ms) 75.75 (±1.50 ms) -10.3ms (12.0%) 👍
tablecmp-append-1k metric base(00a13cc) target(4df6162) trend
benchmark-table-component/append/1k duration 215.90 (±13.20 ms) 236.35 (±11.25 ms) +20.4ms (9.5%) 👎
tablecmp-clear-1k metric base(00a13cc) target(4df6162) trend
benchmark-table-component/clear/1k duration 11.40 (±1.30 ms) 11.80 (±1.40 ms) +0.4ms (3.5%) 👌
tablecmp-create-10k metric base(00a13cc) target(4df6162) trend
benchmark-table-component/create/10k duration 1656.90 (±12.90 ms) 1691.65 (±10.10 ms) +34.8ms (2.1%) 👎
tablecmp-create-1k metric base(00a13cc) target(4df6162) trend
benchmark-table-component/create/1k duration 198.90 (±4.80 ms) 205.35 (±4.75 ms) +6.5ms (3.2%) 👎
tablecmp-update-10th-1k metric base(00a13cc) target(4df6162) trend
benchmark-table-component/update-10th/1k duration 69.35 (±4.25 ms) 69.35 (±3.95 ms) 0.0ms (0.0%) 👌
wc-append-1k metric base(00a13cc) target(4df6162) trend
benchmark-table-wc/append/1k duration 231.25 (±9.40 ms) 243.90 (±9.05 ms) +12.7ms (5.5%) 👎
wc-clear-1k metric base(00a13cc) target(4df6162) trend
benchmark-table-wc/clear/1k duration 22.15 (±2.55 ms) 21.90 (±2.40 ms) -0.3ms (1.1%) 👌
wc-create-10k metric base(00a13cc) target(4df6162) trend
benchmark-table-wc/create/10k duration 1685.95 (±52.35 ms) 1875.70 (±27.05 ms) +189.8ms (11.3%) 👎
wc-create-1k metric base(00a13cc) target(4df6162) trend
benchmark-table-wc/create/1k duration 206.80 (±5.20 ms) 216.35 (±3.05 ms) +9.5ms (4.6%) 👎
wc-update-10th-1k metric base(00a13cc) target(4df6162) trend
benchmark-table-wc/update-10th/1k duration 75.05 (±6.25 ms) 72.75 (±5.95 ms) -2.3ms (3.1%) 👌

@caridy
Copy link
Contributor Author

caridy commented Oct 25, 2018

@diervo not sure how to justify that regression, this PR removes two conditions in prod mode. It should be faster :), and it is not about wc, but for any rendering process.

@ekashida
Copy link
Member

@diervo We need to hold off on this one until after LGC finishes their long-awaited release. This change also needs to go in with 16867957.

@ekashida
Copy link
Member

ekashida commented Nov 2, 2018

The necessary downstream changes have been made but we probably want to release this separately from the big CSS changes since it's always possible (though unlikely) that someone added a new render method that returns undefined.

@caridy
Copy link
Contributor Author

caridy commented Nov 5, 2018

ready to roll

@diervo diervo merged commit c6556de into master Nov 5, 2018
@diervo diervo deleted the caridy/enforce-render-templates branch November 5, 2018 22:11
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.

4 participants