Skip to content

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

Open
tobiasso85 wants to merge 9 commits intov2from
clean-css
Open

[INTERNAL] Use clean-css for compression instead of less.js with compress#360
tobiasso85 wants to merge 9 commits intov2from
clean-css

Conversation

@tobiasso85
Copy link
Contributor

@tobiasso85 tobiasso85 commented Oct 29, 2019 •
edited
Loading

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.

@tobiasso85 tobiasso85 self-assigned this Oct 29, 2019
@RandomByte
Copy link
Member

RandomByte commented Oct 29, 2019

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
Copy link

coveralls commented Oct 30, 2019 •
edited
Loading

Coverage Status

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

@tobiasso85
Copy link
Contributor Author

tobiasso85 commented Oct 30, 2019 •
edited
Loading

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:

@RandomByte
Copy link
Member

RandomByte commented Oct 30, 2019 •
edited
Loading

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
Copy link
Member

RandomByte commented Nov 5, 2019

As discussed, closing this PR.

@RandomByte RandomByte closed this Nov 5, 2019
@RandomByte RandomByte deleted the clean-css branch November 5, 2019 09:16
@RandomByte RandomByte restored the clean-css branch January 16, 2020 14:23
@RandomByte
Copy link
Member

RandomByte commented Jan 16, 2020

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.

@RandomByte RandomByte reopened this Jan 16, 2020
@RandomByte
Copy link
Member

RandomByte commented Jan 16, 2020 •
edited
Loading

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.

@tobiasso85
[INTERNAL] adjusted tests
403c0a8 
"/* Inline theming parameters */" comment is
hard coded in less-openui5, but should not
be present when compressed.
@tobiasso85
[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 tobiasso85 changed the title [INTERNAL] introduce clean css [INTERNAL] Use clean-css for compression instead of less.js compression Jan 17, 2020
@tobiasso85 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
[INTERNAL] add cssOptimizer test
9e0a24e 
Increases coverage
@tobiasso85
Use default clean-css options
1ca6d4c 
Add color optimization example to compressiontheme
test.
@tobiasso85
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
RandomByte reviewed Jan 20, 2020
View reviewed changes
lib/processors/themeBuilder.js
* @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.
Copy link
Member

@RandomByte RandomByte Jan 20, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tobiasso85 tobiasso85 Jan 21, 2020

Choose a reason for hiding this comment

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

done with 75c54b9

@tobiasso85 tobiasso85 requested a review from RandomByte January 22, 2020 06:51
@RandomByte RandomByte changed the base branch from master to v2 November 3, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matz3 matz3 Awaiting requested review from matz3

@svbender svbender Awaiting requested review from svbender

@RandomByte RandomByte Awaiting requested review from RandomByte

At least 1 approving review is required to merge this pull request.

Assignees

@tobiasso85 tobiasso85

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tobiasso85 @RandomByte @coveralls