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(lwc-compiler): invalid CSS generation when minified is enabled #552

Merged
merged 5 commits into from
Aug 1, 2018

Conversation

pmdartus
Copy link
Member

@pmdartus pmdartus commented Jul 31, 2018

Details

This PR fixes an issue where the compiler was producing an invalid javascript when the minified flag is on.

In order to prevent this, we now run the minifier first and then apply the LWC transformation. Since the LWC transformation produces an invalid CSS (but valid javascript!), the minifier wasn't able to parse the values properly and generates an invalid CSS output.

Note: We need to ensure in the future that the LWC postcss transform is always the last one to run.

Does this PR introduce a breaking change?

  • Yes
  • No

@pmdartus pmdartus requested review from diervo and apapko July 31, 2018 22:02
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: b77df3a | Target commit: e60ba3b

lwc-engine-benchmark

table-append-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table/append/1k duration 142.60 (± 3.90 ms) 145.70 (± 5.10 ms) -2.17% 👎
table-clear-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table/clear/1k duration 11.40 (± 0.70 ms) 11.60 (± 0.60 ms) -1.75% 👌
table-create-10k metric base(b77df3a) target(e60ba3b) trend
benchmark-table/create/10k duration 839.65 (± 3.45 ms) 833.90 (± 6.20 ms) 0.68% 👍
table-create-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table/create/1k duration 102.20 (± 1.30 ms) 101.00 (± 2.20 ms) 1.17% 👌
table-update-10th-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table/update-10th/1k duration 84.90 (± 5.00 ms) 86.90 (± 5.40 ms) -2.36% 👌
tablecmp-append-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table-component/append/1k duration 225.90 (± 4.50 ms) 224.10 (± 4.70 ms) 0.80% 👍
tablecmp-clear-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table-component/clear/1k duration 34.60 (± 2.00 ms) 34.70 (± 2.30 ms) -0.29% 👌
tablecmp-create-10k metric base(b77df3a) target(e60ba3b) trend
benchmark-table-component/create/10k duration 1528.70 (± 12.60 ms) 1507.90 (± 8.90 ms) 1.36% 👍
tablecmp-create-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table-component/create/1k duration 173.00 (± 3.30 ms) 170.40 (± 3.90 ms) 1.50% 👍
tablecmp-update-10th-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table-component/update-10th/1k duration 77.60 (± 3.60 ms) 76.20 (± 5.00 ms) 1.80% 👌
wc-append-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table-wc/append/1k duration 264.10 (± 6.60 ms) 254.60 (± 9.30 ms) 3.60% 👍
wc-clear-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table-wc/clear/1k duration 35.30 (± 0.95 ms) 34.70 (± 1.30 ms) 1.70% 👌
wc-create-10k metric base(b77df3a) target(e60ba3b) trend
benchmark-table-wc/create/10k duration 1995.20 (± 8.60 ms) 1966.00 (± 6.30 ms) 1.46% 👍
wc-create-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table-wc/create/1k duration 211.60 (± 5.40 ms) 212.50 (± 4.00 ms) -0.43% 👌
wc-update-10th-1k metric base(b77df3a) target(e60ba3b) trend
benchmark-table-wc/update-10th/1k duration 73.60 (± 6.80 ms) 74.20 (± 3.80 ms) -0.82% 👌


// The minification plugin should be the first plugin to run.
// The LWC plugins produces invalid CSS since it transforms all the var function with actual
Copy link
Collaborator

Choose a reason for hiding this comment

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

plugin - no 's'.
Typo - mification
I think we can either remove the first sentence or the last one since they both kind of are saying the same thing

@pmdartus pmdartus merged commit f19ebb1 into master Aug 1, 2018
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: d0cf318 | Target commit: 2dcf7df

lwc-engine-benchmark

table-append-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table/append/1k duration 145.60 (± 5.30 ms) 144.10 (± 2.80 ms) 1.03% 👌
table-clear-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table/clear/1k duration 11.80 (± 0.40 ms) 11.70 (± 0.60 ms) 0.85% 👌
table-create-10k metric base(d0cf318) target(2dcf7df) trend
benchmark-table/create/10k duration 841.40 (± 5.10 ms) 840.50 (± 5.40 ms) 0.11% 👌
table-create-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table/create/1k duration 100.50 (± 2.50 ms) 102.50 (± 2.30 ms) -1.99% 👌
table-update-10th-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table/update-10th/1k duration 82.60 (± 4.40 ms) 87.10 (± 5.20 ms) -5.45% 👌
tablecmp-append-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table-component/append/1k duration 224.00 (± 4.70 ms) 224.00 (± 5.00 ms) 0.00% 👌
tablecmp-clear-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table-component/clear/1k duration 34.40 (± 2.55 ms) 34.70 (± 1.90 ms) -0.87% 👌
tablecmp-create-10k metric base(d0cf318) target(2dcf7df) trend
benchmark-table-component/create/10k duration 1556.60 (± 7.10 ms) 1546.30 (± 9.60 ms) 0.66% 👍
tablecmp-create-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table-component/create/1k duration 168.70 (± 3.40 ms) 171.60 (± 6.10 ms) -1.72% 👌
tablecmp-update-10th-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table-component/update-10th/1k duration 75.90 (± 4.60 ms) 75.80 (± 4.90 ms) 0.13% 👌
wc-append-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table-wc/append/1k duration 260.90 (± 10.10 ms) 261.10 (± 9.70 ms) -0.08% 👌
wc-clear-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table-wc/clear/1k duration 35.80 (± 1.40 ms) 35.90 (± 1.40 ms) -0.28% 👌
wc-create-10k metric base(d0cf318) target(2dcf7df) trend
benchmark-table-wc/create/10k duration 2015.30 (± 10.60 ms) 1996.90 (± 9.10 ms) 0.91% 👍
wc-create-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table-wc/create/1k duration 209.70 (± 3.70 ms) 211.90 (± 5.20 ms) -1.05% 👌
wc-update-10th-1k metric base(d0cf318) target(2dcf7df) trend
benchmark-table-wc/update-10th/1k duration 74.70 (± 3.70 ms) 75.50 (± 3.60 ms) -1.07% 👌

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: d0cf318 | Target commit: c947d12

lwc-engine-benchmark

table-append-1k metric base(d0cf318) target(c947d12) trend
benchmark-table/append/1k duration 145.60 (± 5.30 ms) 146.00 (± 5.30 ms) -0.27% 👌
table-clear-1k metric base(d0cf318) target(c947d12) trend
benchmark-table/clear/1k duration 11.80 (± 0.40 ms) 11.90 (± 0.70 ms) -0.85% 👌
table-create-10k metric base(d0cf318) target(c947d12) trend
benchmark-table/create/10k duration 841.40 (± 5.10 ms) 855.20 (± 3.90 ms) -1.64% 👎
table-create-1k metric base(d0cf318) target(c947d12) trend
benchmark-table/create/1k duration 100.50 (± 2.50 ms) 102.10 (± 1.80 ms) -1.59% 👌
table-update-10th-1k metric base(d0cf318) target(c947d12) trend
benchmark-table/update-10th/1k duration 82.60 (± 4.40 ms) 89.30 (± 4.60 ms) -8.11% 👌
tablecmp-append-1k metric base(d0cf318) target(c947d12) trend
benchmark-table-component/append/1k duration 224.00 (± 4.70 ms) 223.50 (± 3.90 ms) 0.22% 👌
tablecmp-clear-1k metric base(d0cf318) target(c947d12) trend
benchmark-table-component/clear/1k duration 34.40 (± 2.55 ms) 33.60 (± 1.70 ms) 2.33% 👌
tablecmp-create-10k metric base(d0cf318) target(c947d12) trend
benchmark-table-component/create/10k duration 1556.60 (± 7.10 ms) 1503.60 (± 10.30 ms) 3.40% 👍
tablecmp-create-1k metric base(d0cf318) target(c947d12) trend
benchmark-table-component/create/1k duration 168.70 (± 3.40 ms) 168.40 (± 3.50 ms) 0.18% 👌
tablecmp-update-10th-1k metric base(d0cf318) target(c947d12) trend
benchmark-table-component/update-10th/1k duration 75.90 (± 4.60 ms) 77.40 (± 3.20 ms) -1.98% 👌
wc-append-1k metric base(d0cf318) target(c947d12) trend
benchmark-table-wc/append/1k duration 260.90 (± 10.10 ms) 262.70 (± 9.50 ms) -0.69% 👌
wc-clear-1k metric base(d0cf318) target(c947d12) trend
benchmark-table-wc/clear/1k duration 35.80 (± 1.40 ms) 35.20 (± 1.60 ms) 1.68% 👌
wc-create-10k metric base(d0cf318) target(c947d12) trend
benchmark-table-wc/create/10k duration 2015.30 (± 10.60 ms) 1970.00 (± 8.70 ms) 2.25% 👍
wc-create-1k metric base(d0cf318) target(c947d12) trend
benchmark-table-wc/create/1k duration 209.70 (± 3.70 ms) 206.90 (± 4.70 ms) 1.34% 👌
wc-update-10th-1k metric base(d0cf318) target(c947d12) trend
benchmark-table-wc/update-10th/1k duration 74.70 (± 3.70 ms) 77.10 (± 2.70 ms) -3.21% 👌

@pmdartus pmdartus deleted the pmdartus/css-minification-issue branch August 1, 2018 00:49
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.

2 participants