csscomb / csscomb.js

CSS coding style formatter
http://csscomb.com/
MIT License
3.29k stars 457 forks source link

Scoped variables cause an error when sort-order is enabled #443

Closed gajus closed 5 years ago

gajus commented 8 years ago
.form {
    $foo: #fff;
    $bar: #fff;
}

Produces:

TypeError: a.match is not a function
    at sortLeftovers (/Users/gajus/Documents/dev/gajus/pragmatist/node_modules/csscomb/lib/options/sort-order.js:244:19)
    at /Users/gajus/Documents/dev/gajus/pragmatist/node_modules/csscomb/lib/options/sort-order.js:334:24
    at Array.sort (native)
    at module.exports.process (/Users/gajus/Documents/dev/gajus/pragmatist/node_modules/csscomb/lib/options/sort-order.js:325:16)
    at Object.Node.map (/Users/gajus/Documents/dev/gajus/pragmatist/node_modules/gonzales-pe/lib/node.js:111:9)
    at /Users/gajus/Documents/dev/gajus/pragmatist/node_modules/gonzales-pe/lib/node.js:117:22
    at Array.forEach (native)
    at Object.Node.map (/Users/gajus/Documents/dev/gajus/pragmatist/node_modules/gonzales-pe/lib/node.js:115:22)
    at /Users/gajus/Documents/dev/gajus/pragmatist/node_modules/gonzales-pe/lib/node.js:117:22
    at Array.forEach (native)
    at Object.Node.map (/Users/gajus/Documents/dev/gajus/pragmatist/node_modules/gonzales-pe/lib/node.js:115:22)
    at /Users/gajus/Documents/dev/gajus/pragmatist/node_modules/csscomb-core/lib/core.js:132:18
    at Array.forEach (native)
    at Object.processTree (/Users/gajus/Documents/dev/gajus/pragmatist/node_modules/csscomb-core/lib/core.js:131:19)
    at processString (/Users/gajus/Documents/dev/gajus/pragmatist/node_modules/csscomb-core/lib/core.js:523:22)
    at /Users/gajus/Documents/dev/gajus/pragmatist/node_modules/csscomb-core/lib/core.js:449:38

I did a bit of debugging:

This is sortLeftovers function from sort-order.js that triggers the error.

var sortLeftovers = function(a, b) {
    var prefixes = ['-webkit-', '-moz-', '-ms-', '-o-', ''];
    var prefixesRegExp = /^(-webkit-|-moz-|-ms-|-o-)(.*)$/;

    // Get property name (i.e. `color`, `-o-animation`):
    a = a.node.get(0).get(0).content;
    b = b.node.get(0).get(0).content;

    console.log(a, b);

The output of console.log is:

[ { type: 'ident', content: 'foo', start: { line: 2, column: 6 } } ]
[ { type: 'ident', content: 'bar', start: { line: 3, column: 6 } } ]

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/29843200-scoped-variables-cause-an-error-when-sort-order-is-enabled?utm_campaign=plugin&utm_content=tracker%2F214563&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F214563&utm_medium=issues&utm_source=github).
nnmrts commented 6 years ago

For me this isn't fixed. Still get this issue when I use variables or at-rules. :/

nnmrts commented 6 years ago

Windows 10, vscode insider build and stable build csscomb 4.2.0

config:

{
    "always-semicolon": true,
    "color-case": "lower",
    "block-indent": "\t",
    "color-shorthand": false,
    "element-case": "lower",
    "eof-newline": true,
    "leading-zero": true,
    "quotes": "double",
    "sort-order-fallback": "abc",
    "space-before-colon": "",
    "space-after-colon": " ",
    "space-before-combinator": " ",
    "space-after-combinator": " ",
    "space-between-declarations": "\n",
    "space-before-opening-brace": " ",
    "space-after-opening-brace": "\n",
    "space-after-selector-delimiter": "\n",
    "space-before-selector-delimiter": "",
    "space-before-closing-brace": "\n",
    "strip-spaces": true,
    "unitless-zero": true,
    "vendor-prefix-align": true,
        "sort-order": [
                // ....
        ]
}

For me, csscomb stops working if the scss file has an @-rule or a scoped variable in it:

// doesn't work
#header {
        width: 100%;
        @include blackBg;
}

// doesn't work
#header {
        width: 100%;
        $headerHeight: 64px;
}

// does work
#header {
        width: 100%;
        height: $headerHeight;
}

// does work
#header {
        width: 100%;
        #{$bg}-color: blue
}

In the cases it doesn't work, csscomb throws me an error:

TypeError: a.match is not a function

When I remove my sort-order property from the config, everything works fine. :)

jdalton commented 5 years ago

This should be fixed in the latest release now that the sortLeftovers uses

a = a.node.first().first().content;
b = b.node.first().first().content;
JiniHendrix commented 5 years ago

still getting this issue

jdalton commented 5 years ago

Hi @JiniHendrix!

Can you create a small repro repo that demonstrates the issue for me to investigate.

JiniHendrix commented 5 years ago

hey @jdalton here's an example repo https://github.com/JiniHendrix/csscomb-sort-break

thanks!

jdalton commented 5 years ago

Thank you @JiniHendrix!

Yep, the issue still persists. Let's fix it together! Would you be up for creating a failing test case in a PR that I can write a patch against?

JiniHendrix commented 5 years ago

ill give it a shot 👍

jdalton commented 5 years ago

Rock! The issue is that because left-overs can be a variety of things, one of those being a Variable node which has content as an array (instead of a string like the Ident node).

JiniHendrix commented 5 years ago

@jdalton looks like there already is a test for it, and it's passing. may need to refactor the tests.

test/options/sort-order/process/test.js line 413

JiniHendrix commented 5 years ago

dont really have the time for it unfortunately, seems like you know how to fix the issue though?

jdalton commented 5 years ago

If you're up for the refactor that would be appreciated too : )

Ah okay, sorry. I posted my initial reply before the newer comments popped in. No worries. Thanks for pin pointing the existing test!

Update:

Patch PR started at #609.

JiniHendrix commented 5 years ago

hey @jdalton thanks for fixing the issue. any idea when this will be published?