fix: Use token for the styling host element instead of tag name #390

pmdartus · 2018-06-07T16:54:43Z

Details

This PR changes the way the host element gets targetted in CSS. Instead of relying on the tag name, the compiler now reuses the token to style the host element.

Fix #383

Does this PR introduce a breaking change?

Yes
No

davidturissini

Minor test update

davidturissini · 2018-06-07T17:00:35Z

packages/lwc-engine/src/framework/__tests__/template.spec.ts

+            const cmp = createElement('x-cmp', { is: Component });
+            document.body.appendChild(cmp);
+
+            expect(querySelectorAll.call(cmp, 'div[token]').length).toBe(2);


use cmp.shadowRoot.querySelectorAll

salesforce-best-lwc-internal · 2018-06-07T17:10:30Z

Benchmark results

Base commit: d1cf4a7 | Target commit: 08ee60e

lwc-engine-benchmark

table-append-1k	metric	base(`d1cf4a7`)	target(`08ee60e`)	trend
benchmark-table/append/1k	duration	142.90 (± 4.00 ms)	142.60 (± 4.35 ms)	0.21% 👌

table-clear-1k	metric	base(`d1cf4a7`)	target(`08ee60e`)	trend
benchmark-table/clear/1k	duration	11.00 (± 0.50 ms)	11.30 (± 0.40 ms)	-2.73% 👌

table-create-10k	metric	base(`d1cf4a7`)	target(`08ee60e`)	trend
benchmark-table/create/10k	duration	834.20 (± 6.50 ms)	832.80 (± 8.70 ms)	0.17% 👌

table-create-1k	metric	base(`d1cf4a7`)	target(`08ee60e`)	trend
benchmark-table/create/1k	duration	100.10 (± 1.85 ms)	100.90 (± 2.10 ms)	-0.80% 👌

table-update-10th-1k	metric	base(`d1cf4a7`)	target(`08ee60e`)	trend
benchmark-table/update-10th/1k	duration	91.60 (± 4.40 ms)	91.00 (± 3.70 ms)	0.66% 👌

tablecmp-append-1k	metric	base(`d1cf4a7`)	target(`08ee60e`)	trend
benchmark-table-component/append/1k	duration	259.80 (± 5.10 ms)	256.40 (± 4.20 ms)	1.31% 👍

tablecmp-clear-1k	metric	base(`d1cf4a7`)	target(`08ee60e`)	trend
benchmark-table/clear/1k	duration	34.65 (± 1.15 ms)	34.60 (± 1.80 ms)	0.14% 👌

tablecmp-create-10k	metric	base(`d1cf4a7`)	target(`08ee60e`)	trend
benchmark-table-component/create/10k	duration	1681.70 (± 9.80 ms)	1684.40 (± 12.90 ms)	-0.16% 👌

tablecmp-create-1k	metric	base(`d1cf4a7`)	target(`08ee60e`)	trend
benchmark-table-component/create/1k	duration	196.00 (± 4.30 ms)	195.80 (± 4.20 ms)	0.10% 👌

tablecmp-update-10th-1k	metric	base(`d1cf4a7`)	target(`08ee60e`)	trend
benchmark-table-component/update-10th/1k	duration	75.40 (± 3.40 ms)	73.90 (± 3.90 ms)	1.99% 👌

salesforce-best-lwc-internal · 2018-06-08T09:14:36Z

Benchmark results

Base commit: 800002c | Target commit: 5541acc

lwc-engine-benchmark

table-append-1k	metric	base(`800002c`)	target(`5541acc`)	trend
benchmark-table/append/1k	duration	143.90 (± 3.30 ms)	146.70 (± 5.30 ms)	-1.95% 👌

table-clear-1k	metric	base(`800002c`)	target(`5541acc`)	trend
benchmark-table/clear/1k	duration	11.60 (± 0.40 ms)	11.35 (± 0.45 ms)	2.16% 👌

table-create-10k	metric	base(`800002c`)	target(`5541acc`)	trend
benchmark-table/create/10k	duration	833.40 (± 4.30 ms)	857.20 (± 4.60 ms)	-2.86% 👎

table-create-1k	metric	base(`800002c`)	target(`5541acc`)	trend
benchmark-table/create/1k	duration	98.40 (± 1.80 ms)	99.80 (± 1.60 ms)	-1.42% 👌

table-update-10th-1k	metric	base(`800002c`)	target(`5541acc`)	trend
benchmark-table/update-10th/1k	duration	85.50 (± 4.45 ms)	87.30 (± 5.40 ms)	-2.11% 👌

tablecmp-append-1k	metric	base(`800002c`)	target(`5541acc`)	trend
benchmark-table-component/append/1k	duration	251.20 (± 4.10 ms)	259.90 (± 4.30 ms)	-3.46% 👎

tablecmp-clear-1k	metric	base(`800002c`)	target(`5541acc`)	trend
benchmark-table/clear/1k	duration	36.90 (± 1.50 ms)	36.40 (± 2.00 ms)	1.36% 👌

tablecmp-create-10k	metric	base(`800002c`)	target(`5541acc`)	trend
benchmark-table-component/create/10k	duration	1705.80 (± 18.40 ms)	1688.30 (± 7.00 ms)	1.03% 👍

tablecmp-create-1k	metric	base(`800002c`)	target(`5541acc`)	trend
benchmark-table-component/create/1k	duration	198.15 (± 4.80 ms)	198.70 (± 4.10 ms)	-0.28% 👌

tablecmp-update-10th-1k	metric	base(`800002c`)	target(`5541acc`)	trend
benchmark-table-component/update-10th/1k	duration	74.70 (± 4.10 ms)	74.80 (± 4.10 ms)	-0.13% 👌

caridy · 2018-06-11T16:58:28Z

packages/lwc-engine/src/framework/template.ts

@@ -65,12 +73,14 @@ function applyTokenToHost(vm: VM, html: Template): void {
        // Remove the token currently applied to the host element if different than the one associated
        // with the current template
        if (!isUndefined(oldToken)) {
-            removeAttribute.call(host, oldToken);
+            const oldHostToken = hostToken(oldToken);


why isn't the token that we use in the engine already using the -host postfix? why does the engine needs to know this?

caridy

I don't think the engine should know how to build such token, it should come from compiler, and the engine should just pass it around.

pmdartus · 2018-06-12T14:35:49Z

@caridy Ready for review, the engine now expect 2 properties on the template, hostToken and shadowToken.

salesforce-best-lwc-internal · 2018-06-12T14:45:35Z

Benchmark results

Base commit: 598d940 | Target commit: 6d7ea46

lwc-engine-benchmark

table-append-1k	metric	base(`598d940`)	target(`6d7ea46`)	trend
benchmark-table/append/1k	duration	144.80 (± 4.40 ms)	149.50 (± 6.70 ms)	-3.25% 👎

table-clear-1k	metric	base(`598d940`)	target(`6d7ea46`)	trend
benchmark-table/clear/1k	duration	11.40 (± 0.30 ms)	11.80 (± 0.40 ms)	-3.51% 👎

table-create-10k	metric	base(`598d940`)	target(`6d7ea46`)	trend
benchmark-table/create/10k	duration	838.90 (± 3.40 ms)	852.10 (± 4.90 ms)	-1.57% 👎

table-create-1k	metric	base(`598d940`)	target(`6d7ea46`)	trend
benchmark-table/create/1k	duration	100.90 (± 1.80 ms)	101.50 (± 1.60 ms)	-0.59% 👌

table-update-10th-1k	metric	base(`598d940`)	target(`6d7ea46`)	trend
benchmark-table/update-10th/1k	duration	91.10 (± 5.80 ms)	91.60 (± 5.90 ms)	-0.55% 👌

tablecmp-append-1k	metric	base(`598d940`)	target(`6d7ea46`)	trend
benchmark-table-component/append/1k	duration	253.50 (± 4.40 ms)	262.80 (± 4.90 ms)	-3.67% 👎

tablecmp-clear-1k	metric	base(`598d940`)	target(`6d7ea46`)	trend
benchmark-table/clear/1k	duration	36.40 (± 1.70 ms)	36.30 (± 2.40 ms)	0.27% 👌

tablecmp-create-10k	metric	base(`598d940`)	target(`6d7ea46`)	trend
benchmark-table-component/create/10k	duration	1714.10 (± 16.60 ms)	1688.30 (± 12.30 ms)	1.51% 👍

tablecmp-create-1k	metric	base(`598d940`)	target(`6d7ea46`)	trend
benchmark-table-component/create/1k	duration	199.90 (± 5.00 ms)	201.30 (± 5.30 ms)	-0.70% 👌

tablecmp-update-10th-1k	metric	base(`598d940`)	target(`6d7ea46`)	trend
benchmark-table-component/update-10th/1k	duration	74.90 (± 4.40 ms)	76.90 (± 4.30 ms)	-2.67% 👌

salesforce-best-lwc-internal · 2018-06-12T16:53:15Z

Benchmark results

Base commit: 231e00d | Target commit: 77f52e1

lwc-engine-benchmark

table-append-1k	metric	base(`231e00d`)	target(`77f52e1`)	trend
benchmark-table/append/1k	duration	147.40 (± 4.60 ms)	149.20 (± 6.30 ms)	-1.22% 👌

table-clear-1k	metric	base(`231e00d`)	target(`77f52e1`)	trend
benchmark-table/clear/1k	duration	11.80 (± 0.30 ms)	11.30 (± 0.50 ms)	4.24% 👍

table-create-10k	metric	base(`231e00d`)	target(`77f52e1`)	trend
benchmark-table/create/10k	duration	847.50 (± 4.50 ms)	846.10 (± 3.40 ms)	0.17% 👌

table-create-1k	metric	base(`231e00d`)	target(`77f52e1`)	trend
benchmark-table/create/1k	duration	101.60 (± 2.00 ms)	101.10 (± 1.50 ms)	0.49% 👌

table-update-10th-1k	metric	base(`231e00d`)	target(`77f52e1`)	trend
benchmark-table/update-10th/1k	duration	89.60 (± 5.90 ms)	86.40 (± 6.20 ms)	3.57% 👌

tablecmp-append-1k	metric	base(`231e00d`)	target(`77f52e1`)	trend
benchmark-table-component/append/1k	duration	255.00 (± 4.40 ms)	259.40 (± 5.50 ms)	-1.73% 👌

tablecmp-clear-1k	metric	base(`231e00d`)	target(`77f52e1`)	trend
benchmark-table/clear/1k	duration	35.60 (± 1.40 ms)	35.70 (± 1.50 ms)	-0.28% 👌

tablecmp-create-10k	metric	base(`231e00d`)	target(`77f52e1`)	trend
benchmark-table-component/create/10k	duration	1722.50 (± 18.10 ms)	1748.00 (± 7.70 ms)	-1.48% 👎

tablecmp-create-1k	metric	base(`231e00d`)	target(`77f52e1`)	trend
benchmark-table-component/create/1k	duration	200.00 (± 3.40 ms)	198.30 (± 4.00 ms)	0.85% 👌

tablecmp-update-10th-1k	metric	base(`231e00d`)	target(`77f52e1`)	trend
benchmark-table-component/update-10th/1k	duration	76.10 (± 4.60 ms)	75.30 (± 2.60 ms)	1.05% 👌

## Details Fix unit test failing due to conflicting merges between #390 and #397 ## Does this PR introduce a breaking change? * [ ] Yes * [X] No

## Details Cleanup old style related code in the compiler: * remove `style` property from the component class. The style injection is now done at evaluation time and doesn't rely on an external system to inject it. (obsolete since #302) * Remove `tagName` parameter from `stylesheet` factory. (obsolete since #390) ## Does this PR introduce a breaking change? * [ ] Yes * [X] No

pmdartus added 3 commits June 7, 2018 15:37

fix: drop usage of tagname for style scoping

348840e

test: fix compiler tests

669c8d3

fix: update engine to inject new host token

08ee60e

pmdartus requested review from caridy and davidturissini June 7, 2018 16:54

davidturissini reviewed Jun 7, 2018

View reviewed changes

test: remove usage of querySelector

5541acc

davidturissini approved these changes Jun 9, 2018

View reviewed changes

caridy reviewed Jun 11, 2018

View reviewed changes

caridy suggested changes Jun 11, 2018

View reviewed changes

pmdartus added 2 commits June 12, 2018 06:53

fix: add shadowToken and hostToken

4e492c7

fix: introduce shadowToken and hostToken in the engine

6d7ea46

caridy approved these changes Jun 12, 2018

View reviewed changes

Merge branch 'master' into pmdartus/style-compilation-simplication

77f52e1

pmdartus merged commit cccc63d into master Jun 12, 2018

pmdartus deleted the pmdartus/style-compilation-simplication branch June 12, 2018 18:11

pmdartus mentioned this pull request Jun 13, 2018

test: fix missing getHostShadowRoot in merged files #405

Merged

2 tasks

pmdartus added a commit that referenced this pull request Jun 13, 2018

test: fix missing getHostShadowRoot in merged files (#405)

65ffad0

## Details Fix unit test failing due to conflicting merges between #390 and #397 ## Does this PR introduce a breaking change? * [ ] Yes * [X] No

pmdartus mentioned this pull request Jun 13, 2018

chore: cleanup old style artifacts in the compiled code #406

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use token for the styling host element instead of tag name #390

fix: Use token for the styling host element instead of tag name #390

pmdartus commented Jun 7, 2018

davidturissini left a comment

davidturissini Jun 7, 2018

salesforce-best-lwc-internal bot commented Jun 7, 2018

salesforce-best-lwc-internal bot commented Jun 8, 2018

caridy Jun 11, 2018

caridy left a comment

pmdartus commented Jun 12, 2018

salesforce-best-lwc-internal bot commented Jun 12, 2018

salesforce-best-lwc-internal bot commented Jun 12, 2018

fix: Use token for the styling host element instead of tag name #390

fix: Use token for the styling host element instead of tag name #390

Conversation

pmdartus commented Jun 7, 2018

Details

Does this PR introduce a breaking change?

davidturissini left a comment

Choose a reason for hiding this comment

davidturissini Jun 7, 2018

Choose a reason for hiding this comment

salesforce-best-lwc-internal bot commented Jun 7, 2018

Benchmark results

lwc-engine-benchmark

salesforce-best-lwc-internal bot commented Jun 8, 2018

Benchmark results

lwc-engine-benchmark

caridy Jun 11, 2018

Choose a reason for hiding this comment

caridy left a comment

Choose a reason for hiding this comment

pmdartus commented Jun 12, 2018

salesforce-best-lwc-internal bot commented Jun 12, 2018

Benchmark results

lwc-engine-benchmark

salesforce-best-lwc-internal bot commented Jun 12, 2018

Benchmark results

lwc-engine-benchmark