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

classd tag doesn't act as promised in README #1

Open
mattvalleycodes opened this issue Apr 14, 2020 · 3 comments
Open

classd tag doesn't act as promised in README #1

mattvalleycodes opened this issue Apr 14, 2020 · 3 comments

Comments

@mattvalleycodes
Copy link

The below code is copy/pasted from README:

const classes = classd`container padding-${{
    lg: width > 1280, 
    md: width > 960 && width < 1280,
    sm: width <= 960
}} margin-0 ${width > 960 && 'blue'} ${width < 960 && 'red'}`;
console.log(classes); // => 'container padding-md margin-0 blue'

padding-md is generated as the result of padding-${{md: width > 960 && width < 1280 }}. In reality, I get padding md!

Node.js example

> const classd = require("classd").default;
undefined
> classd`foo-${{bar: true}}`;
'foo- bar' 
>

Unit test

and below test should technically speaking pass but it fails.

    it.only ('should use the provided prefix', function () {
        return assert.equal(
            classd`button--${{ blue: true }}`,
            'button--blue'
        );
    });

with the below error:

    1) should use the provided prefix


  0 passing (6ms)
  1 failing

  1) classd
       should use the provided prefix:

      AssertionError [ERR_ASSERTION]: 'button-- blue' == 'button--blue'
      + expected - actual

      -button-- blue
      +button--blue

      at Context.<anonymous> (tests/classd.js:95:23)
      at processImmediate (internal/timers.js:456:21)



npm ERR! Test failed.  See above for more details.

@mattvalleycodes
Copy link
Author

This is a WIP but I think updating classd to the below will fix the problem:

function classd(strings, ...values) {
    var res = '';
    for (var i = 0; i < strings.length; i++) {
        if (typeof values[i] === "object" && !Array.isArray(values[i]) && strings[i][strings.length - 1] !== " ") {
          const prefix = strings[i].trim().split(" ").pop();
          res += names.call(null, values[i]).trim().split(" ").reduce(function(acc, value) {
            return acc + " " + prefix + value;
          }, "");
        } else {
        res += strings[i];
        res += names.call(null, values[i]);
      }
    }
    return trim.call(null, res);
}

All of the existing tests, as well as the new one passed with the above change in place.

@GnsP
Copy link
Owner

GnsP commented Apr 14, 2020

@mattvalleycodes , my apologies for this confusing scenario, the behaviour is intentional; but I had not updated the README. Here is the reasoning behind this behaviour :

Consider the case

classd`x-${{ y: true, z: true }}`

classd`${{ a: true, b: true }}-${{ x: true, y: true }}`

In this case the values should be x-y x-z and a-x a-y b-x b-y, if we follow the prefix rule. However I thought that may be a bit confusing for the users because of the exponential growth of classnames with respect to the growth in number of prefixes. Also such exponential growth makes it difficult to debug the classnames. So, I made the objects to resolve to list of classnames without the prefixing.

The recommended way for the current version is to prefix the individual object keys. I will update the README accordingly. In the next version, I will support prefixing in a safe manner.

Thanks for reporting this.

@GnsP
Copy link
Owner

GnsP commented Apr 14, 2020

This is a WIP but I think updating classd to the below will fix the problem:

function classd(strings, ...values) {
    var res = '';
    for (var i = 0; i < strings.length; i++) {
        if (typeof values[i] === "object" && !Array.isArray(values[i]) && strings[i][strings.length - 1] !== " ") {
          const prefix = strings[i].trim().split(" ").pop();
          res += names.call(null, values[i]).trim().split(" ").reduce(function(acc, value) {
            return acc + " " + prefix + value;
          }, "");
        } else {
        res += strings[i];
        res += names.call(null, values[i]);
      }
    }
    return trim.call(null, res);
}

All of the existing tests, as well as the new one passed with the above change in place.

This approach would work for your testcase. But I think we can optimise it further.

One key consideration in the classd package is the speed. classd can be called thousands (even millions) of times in a real project, an it can be called a significant number of times everytime a component renders. Therefore every little performance gain that we can achieve matters a lot.

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

No branches or pull requests

2 participants