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

complex units are broken via C-API #1149

Closed
chriseppstein opened this issue Apr 29, 2015 · 10 comments
Closed

complex units are broken via C-API #1149

chriseppstein opened this issue Apr 29, 2015 · 10 comments

Comments

@chriseppstein
Copy link
Contributor

Issue #1143 got me thinking about numbers with units and I found another bug with a similar characteristic. That is, if you pass the value you get from Sass back through the C-API then it has changed.

Here's my test case:

  it("should support complex units", function (done) {
    var input = "#test1 { area: 1px * 2px / 1px;}\n" +
                "         area-fn: testing(1px * 2px) / 1px; }\n" +
                "#test2 { ratio: 20px / 2% * 1%;}\n" +
                "         ratio-fn: testing(20px / 2%) * 1%; }";
    var options = {
      data: input,
      functions: {
        "testing($value)": function(value) {
          console.log(value.getUnit());
          return value;
        }
      }
    };
    var expected = "#test1 { area: 1px; area-fn: 1px; }\n" +
                   "#test2 { ratio: 10px; ratio-fn: 10px; }\n";
    testutils.assertCompiles(options, expected, done);
  });

The complex units are passed as a string of the form numerator_unit1*numerator_unit1/denominator_unit1*denominator_unit2

However, if you pass this string back it becomes a single (illegal) unit having that same appearance but in no way acting like a complex unit as I get a compilation failure about illegal units instead of those units canceling as they should.

@chriseppstein
Copy link
Contributor Author

Sass units are a pain in the ass. The ruby API exposes two arrays numerator_units and denominator_units for each number.

I wrote a unit parser to decompose a unit string (same form as above) into two arrays. It's part of the value helpers api.
https://github.com/sass/sass/blob/stable/lib/sass/script/value/helpers.rb

@mgreter
Copy link
Contributor

mgreter commented Apr 30, 2015

Probably related #1049 ... to funny, your issue is #1149 :)

@mgreter mgreter mentioned this issue Apr 30, 2015
9 tasks
@chriseppstein
Copy link
Contributor Author

I don't think so. It seemed that complex units worked fine except when my custom function was involved.

Hunt & pecked on my iPhone... Sorry if it's brief!

On Apr 29, 2015, at 5:48 PM, Marcel Greter notifications@github.com wrote:

Probably related #1049 ... to funny, your issue is 1149 :)


Reply to this email directly or view it on GitHub.

@mgreter
Copy link
Contributor

mgreter commented Jul 14, 2015

PR #1338 should bring a big improvement in regard of value handling and the C-API.
Probably still not 100% complete, but we're slowly getting there!

Does this problem still exist? Can you give some other examples how it fails for you? THX!

@saper
Copy link
Member

saper commented Aug 25, 2015

Having removed extra braces in input:

    var sass = require('node-sass');

    var input = "#test1 { area: 1px * 2px / 1px;\n" +
                "         area-fn: testing(1px * 2px) / 1px; }\n" +
                "#test2 { ratio: 20px / 2% * 1%;\n" +
                "         ratio-fn: testing(20px / 2%) * 1%; }";

    var options = {
      data: input,
      functions: {
        "testing($value)": function(value) {
          console.log(value.getUnit());
          return value;
        }
      }
    };
    var expected = "#test1 { area: 1px; area-fn: 1px; }\n" +
                   "#test2 { ratio: 10px; ratio-fn: 10px; }\n";


    sass.renderSync(options);
    console.log(options.result.css.toString());

I am getting:

px*px
px/%
#test1 {
  area: 2px;
  area-fn: 2px; }

#test2 {
  ratio: 10px;
  ratio-fn: 10px; }

so units are still strings, but they do work here and back.

Tested with node-sass master sass/node-sass@7c15014, libsass master as of Aug 20th 0ae11a4 (also 66f4e68)

@HamptonMakes
Copy link
Member

It's nice when you realize you fixed a bug without meaning to.

On Tue, Aug 25, 2015 at 3:10 PM, Marcin Cieślak notifications@github.com
wrote:

Having removed extra braces in input:

    var sass = require('node-sass');
    var input = "#test1 { area: 1px * 2px / 1px;\n" +
                "         area-fn: testing(1px * 2px) / 1px; }\n" +
                "#test2 { ratio: 20px / 2% * 1%;\n" +
                "         ratio-fn: testing(20px / 2%) * 1%; }";
    var options = {
      data: input,
      functions: {
        "testing($value)": function(value) {
          console.log(value.getUnit());
          return value;
        }
      }
    };
    var expected = "#test1 { area: 1px; area-fn: 1px; }\n" +
                   "#test2 { ratio: 10px; ratio-fn: 10px; }\n";

    sass.renderSync(options);
    console.log(options.result.css.toString());

I am getting:

px*px
px/%
#test1 {
  area: 2px;
  area-fn: 2px; }
#test2 {
  ratio: 10px;
  ratio-fn: 10px; }

so units are still strings, but they do work here and back.

Tested with node-sass master sass/node-sass@7c15014, libsass master as of Aug 20th 0ae11a4

Reply to this email directly or view it on GitHub:
#1149 (comment)

@mgreter
Copy link
Contributor

mgreter commented Aug 31, 2015

I'm going to close this since there has been a lot of work done regarding C-API and value and number handling. IMHO you should be able to exchange numbers with complex units with libsass now. We also expose a sass_op function now that can be used to access all binary operations.

Units are indeed expected to be exchanged as strings. We do keep them as separate vectors for nominators and denominators in C++, but we need to convert this data somehow for the C-API anyway. Since C has no real support for arrays with strings of variable length, it doesn't make a big difference for the C level if we pass the unit in the current string form or use two "null terminated pseudo arrays". So if you think it makes sense to have these arrays available for numbers in node-sass, I would suggest to implement that in the binding code directly.

Please open a new issue if you find more problems! Thanks!

@mgreter mgreter closed this as completed Aug 31, 2015
@saper
Copy link
Member

saper commented Aug 31, 2015

I agree. I have an idea: what about storing units (via C API or maybe internally as well) as a fixed-length array of all incompatible units just with their exponents? Sure, you waste few bytes almost every time, but even if we would have implemented all units from Système International it would be only an array of 7 bytes.

@mgreter
Copy link
Contributor

mgreter commented Aug 31, 2015

@saper sass accepts anything as units, not just the ones specified in css, so the max length is unknown. I don't see any benefit in this, as parsing the given unit string is as straight forward in C as any other exchange format I could possibly think of (instead of null terminated, it is '*' and '/' terminated now). Btw. the following are valid units for sass:

test: (1\65 _em--_--e0 - 2\65 _em--_--e0);

@saper
Copy link
Member

saper commented Aug 31, 2015

@saper sass accepts anything as units, not just the ones specified in
css, so the max length is unknown. I don't see any benefit in this, as
parsing the given unit string is as straight forward in C as any other
exchange format I could possibly think of (instead of null terminated,
it is '*' and '/' terminated now). Btw. the following are valid units
for sass:

test: (1\65 _em--_--e0 - 2\65 _em--_--e0);

Excellent, thanks for pointing this out :)

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

No branches or pull requests

5 participants