Skip to content

fix(@angular/cli): use safer clean-css optimization level#10101

Merged
clydin merged 2 commits intoangular:1.7.xfrom
iamchucky:fix-clean-css-optimize-level
Apr 5, 2018
Merged

fix(@angular/cli): use safer clean-css optimization level#10101
clydin merged 2 commits intoangular:1.7.xfrom
iamchucky:fix-clean-css-optimize-level

Conversation

@iamchucky
Copy link

@iamchucky iamchucky commented Mar 28, 2018

Target: 1.7.x

@clydin
Copy link
Member

clydin commented Mar 29, 2018

Please open an issue following the template guidelines and describing the problem that this would address.

@splincode
Copy link

If you are sure of this, you need to write and attach tests

@iamchucky
Copy link
Author

@clydin please take a look at the issue #10186

Repo to reproduce

@dominikg
Copy link

dominikg commented Apr 5, 2018

i think disabling all level 2 optimizations is overkill. This sounds like an issue with shorthand merging for non-adjacent properties. clean-css level2 optimizations contain some options that read like they control exactly that.

@iamchucky
Copy link
Author

@dominikg for my use case mentioned in the issue #10186 , yes, setting mergeIntoShorthands: false will resolve it.

Level 2 optimizations operate at rules or multiple properties level, e.g. can remove duplicate rules, remove properties redefined further down a stylesheet, or restructure rules by moving them around.

My original rationale was trying to reduce the surprises of the resulting CSS by using level 1 instead of the more aggressive level 2 as there may be other edge cases that are not just caused by mergeIntoShorthands.

I am open to suggestions and would like to know what the team thinks.

@clydin
Copy link
Member

clydin commented Apr 5, 2018

This appears to be a defect with the mergeIntoShorthands optimization. For cases such as this, we generally will disable the specific rule with a comment explaining why and a link to an issue within the project in question (clean-css in this case). Once the defect is corrected, the minimum version for the package should be increased to the fixed version and the disabled rule can then be removed.

@iamchucky Can you update the PR in this regard?

@iamchucky
Copy link
Author

@clydin done, and created an issue clean-css/clean-css#1022

@clydin clydin merged commit e48d545 into angular:1.7.x Apr 5, 2018
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants