[INTERNAL] Use clean-css for compression instead of less.js with compress #360

tobiasso85 · 2019-10-29T15:56:29Z

Introduce separate CSS optimization processor such that it can be used standalone, e.g. when serving files in ui5 server and to have a better css compression

Fixes shortcomings of less.js css compression. Compiler option compress was deprecated and produced invalid css when using calc with 0px.

RandomByte · 2019-10-29T18:35:45Z

Could you add some context for those who where not involved in the discussion? (like me 🥺)

Questions:
I guess the CSS compression gets better through the use of clean-css? And this is in addition to the less internal compression? Or should it replace it?

Is file size reduction the only motivation for this change?

What is the reason for for having two parameters, compress and compressJSON? Isn't this more of a "mode"? I.e. does it make sense to ever set both to true?

coveralls · 2019-10-30T08:21:04Z

Coverage increased (+0.06%) to 90.985% when pulling 75c54b9 on clean-css into ab6562f on master.

tobiasso85 · 2019-10-30T08:46:11Z

Could you add some context for those who where not involved in the discussion? (like me 🥺)

Questions:
I guess the CSS compression gets better through the use of clean-css? And this is in addition to the less internal compression? Or should it replace it?

Is file size reduction the only motivation for this change?

What is the reason for for having two parameters, compress and compressJSON? Isn't this more of a "mode"? I.e. does it make sense to ever set both to true?

You're right, let's discuss this again with all. This is also more trying out this solution. It was taken into account because:

using less to compress is in newer versions deprecated: http://lesscss.org/usage/#less-options-compress-deprecated-
clean-css has more options and we can stay backward compatible with the color shortening (less compression shortens hex colors, e.g. from #000000 to #000 which breaks openui5 tests): https://github.com/jakubpawlowicz/clean-css#compatibility-modes

RandomByte · 2019-10-30T13:20:27Z

Scenario: Building `sap.m` on OpenUI5

master: Average of 19,007 seconds over 10 runs
clean-css (1f8f7ed): Average of 20,522 seconds over 10 runs
Difference: 1,515 seconds

Command: ui5 build
Benchmark tool: hyperfine with one warmup
Hardware: MacBook Pro 2017 - i7 2,9GHz/16GB

RandomByte · 2019-11-05T09:16:23Z

As discussed, closing this PR.

RandomByte · 2020-01-16T14:24:35Z

As it turnes out, the less compression feature has serious bugs. See SAP/less-openui5#112

Switching to clean-css might resolve those with little performance penalty.

WIP

RandomByte · 2020-01-16T15:16:44Z

Rebased onto current master and resolved conflicts. There are still failing tests (different CSS output than expected) however, an expert should check for the correctness of the new output.

"/* Inline theming parameters */" comment is hard coded in less-openui5, but should not be present when compressed.

Add test to ensure css statements are valid with regards to "calc"-function being called with "0px". Compression should not transform "0px" to "0" within calc statement.

Increases coverage

Add color optimization example to compressiontheme test.

compress parameter should not be used but rather a combination of cssOptimizer on CSS resources and compressJSON parameter should be used instead.

RandomByte · 2020-01-20T12:43:20Z

lib/processors/themeBuilder.js

@@ -30,9 +30,12 @@ class ThemeBuilder {
 	 * @param {module:@ui5/fs.Resource[]} resources Library files
 	 * @param {Object} [options] Build options
 	 * @param {boolean} [options.compress=false] Compress build output (CSS / JSON)
+	 *						DEPRECATED: Use [cssOptimizer]{@link module:@ui5/builder.processors.cssOptimizer} to compress CSS output.


A warning log in the code would also be reasonable. Similar to https://github.com/SAP/ui5-fs/blob/master/lib/resourceFactory.js#L72

done with 75c54b9

tobiasso85 requested review from matz3, RandomByte and svbender October 29, 2019 15:56

tobiasso85 self-assigned this Oct 29, 2019

RandomByte closed this Nov 5, 2019

RandomByte deleted the clean-css branch November 5, 2019 09:16

RandomByte restored the clean-css branch January 16, 2020 14:23

RandomByte reopened this Jan 16, 2020

tobiasso85 added 3 commits January 16, 2020 15:48

[INTERNAL] introduce clean css

3aa1678

WIP

[INTERNAL] added compressJSON flag

ce1236d

[INTERNAL] fix tests

fc93f44

RandomByte force-pushed the clean-css branch from 1f8f7ed to fc93f44 Compare January 16, 2020 15:15

tobiasso85 added 2 commits January 17, 2020 10:50

[INTERNAL] adjusted tests

403c0a8

"/* Inline theming parameters */" comment is hard coded in less-openui5, but should not be present when compressed.

[INTERNAL] add compressiontheme test

664640b

Add test to ensure css statements are valid with regards to "calc"-function being called with "0px". Compression should not transform "0px" to "0" within calc statement.

tobiasso85 changed the title ~~[INTERNAL] introduce clean css~~ [INTERNAL] Use clean-css for compression instead of less.js compression Jan 17, 2020

tobiasso85 changed the title ~~[INTERNAL] Use clean-css for compression instead of less.js compression~~ [INTERNAL] Use clean-css for compression instead of less.js with compress Jan 17, 2020

tobiasso85 added 3 commits January 17, 2020 14:00

[INTERNAL] add cssOptimizer test

9e0a24e

Increases coverage

Use default clean-css options

1ca6d4c

Add color optimization example to compressiontheme test.

Add deprecation info for compress option

aec319d

compress parameter should not be used but rather a combination of cssOptimizer on CSS resources and compressJSON parameter should be used instead.

RandomByte reviewed Jan 20, 2020

View reviewed changes

Add deprecation warning if compress option is used

75c54b9

tobiasso85 requested a review from RandomByte January 22, 2020 06:51

RandomByte changed the base branch from master to v2 November 3, 2022 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INTERNAL] Use clean-css for compression instead of less.js with compress #360

[INTERNAL] Use clean-css for compression instead of less.js with compress #360

tobiasso85 commented Oct 29, 2019 •

edited

Loading

RandomByte commented Oct 29, 2019

coveralls commented Oct 30, 2019 •

edited

Loading

tobiasso85 commented Oct 30, 2019 •

edited

Loading

RandomByte commented Oct 30, 2019 •

edited

Loading

RandomByte commented Nov 5, 2019

RandomByte commented Jan 16, 2020

RandomByte commented Jan 16, 2020 •

edited

Loading

RandomByte Jan 20, 2020

tobiasso85 Jan 21, 2020

[INTERNAL] Use clean-css for compression instead of less.js with compress #360

Are you sure you want to change the base?

[INTERNAL] Use clean-css for compression instead of less.js with compress #360

Conversation

tobiasso85 commented Oct 29, 2019 • edited Loading

RandomByte commented Oct 29, 2019

coveralls commented Oct 30, 2019 • edited Loading

tobiasso85 commented Oct 30, 2019 • edited Loading

RandomByte commented Oct 30, 2019 • edited Loading

Scenario: Building sap.m on OpenUI5

RandomByte commented Nov 5, 2019

RandomByte commented Jan 16, 2020

RandomByte commented Jan 16, 2020 • edited Loading

RandomByte Jan 20, 2020

Choose a reason for hiding this comment

tobiasso85 Jan 21, 2020

Choose a reason for hiding this comment

tobiasso85 commented Oct 29, 2019 •

edited

Loading

coveralls commented Oct 30, 2019 •

edited

Loading

tobiasso85 commented Oct 30, 2019 •

edited

Loading

RandomByte commented Oct 30, 2019 •

edited

Loading

Scenario: Building `sap.m` on OpenUI5

RandomByte commented Jan 16, 2020 •

edited

Loading