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

feat(css/minifier) Remove useless zeroes #6196

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ghostd
Copy link
Contributor

@ghostd ghostd commented Oct 18, 2022

Description:

Remove useless zeroes:
calc(3px + 10em -3px) => 10em

BREAKING CHANGE:

Related issue (if exists):

@@ -93,6 +93,7 @@ struct CalcSumContext {
impl CalcSumContext {
pub fn fold(&mut self, calc_sum: &mut CalcSum) {
self.nested_fold(None, calc_sum);
self.remove_zeroes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need extra loop? Can we solve it in nested_fold, because it is just sum with another sign, don't forget we should implement logic looking at all future math functions (not only calc)

self.expressions.remove(term_index - 1);
true
}
}
Copy link
Contributor

@alexander-akait alexander-akait Oct 18, 2022

Choose a reason for hiding this comment

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

Please read my comment above, we should not add extra loops, these things can be solved in one loop and expression, any plus or minus with 0 just return value itself or value with anither sign

Copy link
Contributor

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

We should refactor out internal logic, no need extra loops, please read a spec - https://www.w3.org/TR/css-values-4/#calc-simplification, finally we should refactor our logic to this

@kdy1 kdy1 self-assigned this Oct 18, 2022
@kdy1 kdy1 added this to the Planned milestone Oct 18, 2022

.class10 {
width: calc(0 - (pi * 3px) - 0px - 4em);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think calc(0 - 0px) is an invaild value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, we should have something like calc(0q - (pi * 3px) - 0px - 4em)

@ghostd
Copy link
Contributor Author

ghostd commented Oct 19, 2022

@alexander-akait do you mean we should refactor the code to strictly follow the spec (and create structures like "Invert" and "Negate" in an intermediate step as defined in the spec, by wrapping the matching AST node)? Or "just" refactor the code to follow the spec as much as possible with our current AST nodes without intermediate transformation ?

@alexander-akait
Copy link
Contributor

Or "just" refactor the code to follow the spec as much as possible with our current AST nodes without intermediate transformation ?

I am fine with it, too

But will be great if you try to, but we can postpone it:

@alexander-akait do you mean we should refactor the code to strictly follow the spec (and create structures like "Invert" and "Negate" in an intermediate step as defined in the spec, by wrapping the matching AST node)?

Not at the parser level, just like PoC (i know it can take a time), because it may turn out that it is faster and more convenient.

@ghostd
Copy link
Contributor Author

ghostd commented Oct 19, 2022

@alexander-akait btw, i just read this from the spec:

It’s important to maintain zero-valued terms, so the calc() doesn’t suddenly "change shape" in the middle of a transition when one of the values happens to have a zero value temporarily.

Do you think it's relevant in our case?

@alexander-akait
Copy link
Contributor

@ghostd And yes and no, look at example:

https://tomhodgins.com/demo/cssom/

and put the code:

.class {
    width: calc(20px + 0px);
}

.class {
    width: calc(20px + 0%);
}

you will see how it works

@ghostd
Copy link
Contributor Author

ghostd commented Oct 19, 2022

Not at the parser level, just like PoC (i know it can take a time), because it may turn out that it is faster and more convenient.

Ok, i'll give a try (in an other poc-branch) to refactor our current implementation like this:

  • swc parse the CSS string (no changes)
  • visit_mut_component_value calls our "simplify calc" function (no changes)
  • transform our calc AST into an intermediate "CSS operator trees" as defined by https://www.w3.org/TR/css-values-4/#parse-a-calculation
  • actually simplify the intermediate tree by following the spec
  • serialize the intermediate tree into the our common AST nodes (i guess unwrapping the structures will be enough)

Is that ok for you?

@ghostd
Copy link
Contributor Author

ghostd commented Oct 19, 2022

you will see how it works

Thanks for the pointer.

So i understood the spec correctly, and this PR is wrong since it transforms width: calc(20px + 0%); into width: calc(20px);... which is wrong. The simplification of calc(20px + 0px) into calc(20px) is already merged.

@alexander-akait
Copy link
Contributor

@ghostd Yes, that is ideal solution ⭐

I think we need more 0 test here, with 0%, 0px, just 0, before/after parens and etc

@kdy1
Copy link
Member

kdy1 commented Dec 8, 2022

@ghostd Would you mind if I take on this?

@ghostd
Copy link
Contributor Author

ghostd commented Dec 8, 2022

@kdy1 of course you can :)

I hope finish the other PR this weekend (i signed for a new position and was busy last weeks).

@kdy1
Copy link
Member

kdy1 commented Dec 8, 2022

Thank you!

@kdy1 kdy1 removed their assignment Jan 15, 2023
@ghostd ghostd requested a review from a team as a code owner April 29, 2024 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants