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

Unitless values should not compare as equal to unitful values #1496

Closed
mirisuzanne opened this Issue Oct 30, 2014 · 26 comments

Comments

Projects
None yet
9 participants
@mirisuzanne

mirisuzanne commented Oct 30, 2014

(Edit after the fact by @nex3)

Tasks:

  • Deprecate existing behavior in stable.
  • Remove behavior from master.

I know the existing behavior was intentional, but it breaks the basic rules of math: if 1px == 1 and 1 == 1em, then 1px == 1em. That sounds like a bug to me. In CSS the actual unit or lack of unit is very important. I can't think of any real use-case where I would be equally happy with or without a unit.

@mirisuzanne mirisuzanne added the Bug label Oct 30, 2014

@lolmaus

This comment has been minimized.

lolmaus commented Oct 30, 2014

I can't think of any real use-case where I would be equally happy with or without a unit.

There is at least one situation: line-height will behave identically for unitless values and ems (inheritance will behave differently though).

Just sayin'. Don't think i'm trying to argue, i fully agree with Eric.

@mirisuzanne

This comment has been minimized.

mirisuzanne commented Oct 30, 2014

(inheritance will behave differently though).

That's exactly my point. As an author, that distinction is huge.

@nex3

This comment has been minimized.

Contributor

nex3 commented Oct 31, 2014

I'm on board with this, although it'll require a deprecation period.

@nex3 nex3 added this to the 4.0 milestone Oct 31, 2014

@mirisuzanne mirisuzanne changed the title from Unitless values should not compare as equal to united values to Unitless values should not compare as equal to unitful values Nov 1, 2014

@mirisuzanne

This comment has been minimized.

mirisuzanne commented Nov 1, 2014

Thanks!

@HugoGiraudel

This comment has been minimized.

Contributor

HugoGiraudel commented Nov 8, 2014

This is where having both == and === would make sense.

1 ==  1px // true
1 === 1px // false

This would allow you to preserve backward compatibility while having a clean and standard way to support @ericam's request.

@yratof

This comment has been minimized.

yratof commented Nov 11, 2014

What would be the work around for this then? At the moment, I'm using $value == 0 for a function, and in this case, 0rem == 0 and 0px == 0 is true

@HugoGiraudel

This comment has been minimized.

Contributor

HugoGiraudel commented Nov 11, 2014

You could use unitless().

@mirisuzanne

This comment has been minimized.

mirisuzanne commented Nov 11, 2014

For True we use a function that compares both value and unit:

@function is-equal($one, $two) {
  @if type-of($one) == number and type-of($two) == number {
    @return $one == $two and unit($one) == unit($two);
  } @else {
    @return $one == $two;
  }
}
@HugoGiraudel

This comment has been minimized.

Contributor

HugoGiraudel commented Nov 11, 2014

@function is-equal($one, $two) {
  @if type-of($one) == "number" and type-of($two) == "number" {
    @return $one == $two and unit($one) == unit($two);
  }

  @return $one == $two;
}

test {
  a: is-equal(1, 1);
  a: is-equal(1, 1px);
  a: is-equal(1, "1");
  a: is-equal("a", "a");
}
@mirisuzanne

This comment has been minimized.

mirisuzanne commented Nov 11, 2014

But it breaks if neither is a number:

test {
  a: is-equal(string, string);   // true
}

// $number: "string" is not a number for `unit'
@mirisuzanne

This comment has been minimized.

mirisuzanne commented Nov 11, 2014

And type is already compared by Sass with a simple == so I don't think the type-check is actually needed (except to make sure only numbers get the unit comparison).

@HugoGiraudel

This comment has been minimized.

Contributor

HugoGiraudel commented Nov 11, 2014

Nice catch. Fixed. Which is what you had in the first place.

@mirisuzanne

This comment has been minimized.

mirisuzanne commented Nov 11, 2014

They look pretty similar now. :)

@yratof

This comment has been minimized.

yratof commented Dec 1, 2014

Strange. The function you've got for is-equal brings up the same error.

The result of 0rem == 0will befalse in future releases of Sass.

Am I missing something here?

@mirisuzanne

This comment has been minimized.

mirisuzanne commented Dec 1, 2014

I wrote that function before there was a deprecation warning to avoid. This adjustment eliminates the warning:

@function _true-is-equal($one, $two) {
  @if type-of($one) == number and type-of($two) == number {
    @if unit($one) == unit($two) {
      @return $one == $two;
    } @else {
      @return false;
    }
  } @else {
    @return $one == $two;
  }
}

dwayne-roberts pushed a commit to dwayne-roberts/beauty-case that referenced this issue Mar 13, 2015

@thertweck

This comment has been minimized.

thertweck commented Apr 4, 2015

Wouldn't this have further effects?

  • If 0 == 0px is false, 0 != 0px should be true
  • If 0 == 0px is false, 0 <= 0px as well as 0 >= 0px should be false
  • Of course 0 < 0px and 0 > 0px is false

Now we have:

0 < 0px, 0 <= 0px, 0 == 0px, 0 >= 0px and 0 > 0px are all false.

This is rather atypical. In general if a number is not smaller or larger than another number they have to be equal.

This may suggest, that unitless numbers should not be compared at all to numbers with units and just like px and % are incompatible, no unit and a unit should be incompatible, too.

That 0 == 0px is false and 0 != 0px is true would be consistent, because 0% == 0px is false and 0% != 0px is true. But 0 < 0px should then be an illegal operation just like 0% < 0px is illegal.

This of course is a massive change, and maybe 0 < 0px and the other comparisons should be deprecated, too.

At the end of the day, this also changes additions and subtractions. 5px + 5 should not be working anymore as well as 5px - 5 because numbers with units and unitless numbers are incompatible.

@nex3

This comment has been minimized.

Contributor

nex3 commented Apr 21, 2015

0 != 0px should definitely return true; this currently doesn't produce a deprecation message, but it should.

The rest of the operations should continue to behave as they do currently. Not only would changing this be a much more serious backwards incompatibility, there's some utility in being able to use a unitless number in a generic way.

nex3 added a commit that referenced this issue May 23, 2015

@Wordius

This comment has been minimized.

Wordius commented Jun 8, 2015

Clearly, I am not as clued up on this as some. How would I fix the following, specifically @if $value == 0 is picking up a warning:

@mixin rems($property, $px-values) {
  // Convert the baseline into rems
  $baseline-rem: $baseline-px / 1rem;
  // Print the first line in pixel values
  #{$property}: $px-values;
  // If there is only one (numeric) value, return the property/value line for it.
  @if type-of($px-values) == "number" {
    #{$property}: $px-values / $baseline-rem;
  }
  @else {
    // Create an empty list that we can dump values into
    $rem-values: unquote("");
    @each $value in $px-values {
      // If the value is zero, return 0
      @if $value == 0 {
        $rem-values: append($rem-values, $value);
      }
      @else {
        $rem-values: append($rem-values, $value / $baseline-rem);
      }
    }
    // Return the property and its list of converted values
    #{$property}: $rem-values;
  }
}
@nex3

This comment has been minimized.

Contributor

nex3 commented Jun 19, 2015

@Wordius assuming $px-values is, as the name indicates, in px, you would do $value == 0px.

@hcatlin

This comment has been minimized.

Member

hcatlin commented Nov 13, 2015

What are the thoughts here on the === unitless comparison? Seems like it might make the deprecation warning have a more useful suggestion. @nex3?

@nex3

This comment has been minimized.

Contributor

nex3 commented Nov 13, 2015

I'm strongly opposed to adding a new operator with such a narrow purpose.

nex3 added a commit to sass/sass-spec that referenced this issue Jun 16, 2017

nex3 added a commit that referenced this issue Jun 16, 2017

Unitless numbers no longer equal numbers with units. (#2318)
Unitless numbers no longer equal numbers with units.

Closes #1496

nex3 added a commit to sass/sass-spec that referenced this issue Jun 16, 2017

@nex3 nex3 closed this Jun 16, 2017

@mirisuzanne

This comment has been minimized.

mirisuzanne commented Jun 16, 2017

🎉

@lunelson

This comment has been minimized.

lunelson commented Jan 4, 2018

Question: how does this change affect the comparable() function, and arithmetic operations on unit/unitless numbers? Is it only about equality checks, or will the following expressions also change?

e.g. comparable(2px, 2) == true; 2px + 2 == 4

@nex3

This comment has been minimized.

Contributor

nex3 commented Jan 5, 2018

Unitless numbers are still considered comparable to numbers with units. This does lead to the unfortunate case where $x >= $y isn't always the same as $x > $y or $x == $y, but I'm not sure I see a great way around that.

For your specific examples:

  • comparable(2px, 2) == true returns true in both the old scheme and the new.
  • 2px + 2 == 4 returned true in the old scheme (because 2px + 2 is 4px and 4px == 4), and will instead return false (because now 4px != 4).
@lunelson

This comment has been minimized.

lunelson commented Jan 5, 2018

Thanks @nex3 ; so I can take it that unit/unitless operations remain a stable core behavior? i.e. 1px + 1 will always be 2px? It's just a question related to the sass-calc PR reference above.

@nex3

This comment has been minimized.

Contributor

nex3 commented Jan 5, 2018

I can take it that unit/unitless operations remain a stable core behavior?

That's correct. There are no plans to change anything but boolean operators (and so far, only ==).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment