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

Hyphen in prefixed values gets parsed as operator node when in loose mode #38

Open
jwilsson opened this Issue Sep 26, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@jwilsson
Collaborator

jwilsson commented Sep 26, 2017

Originally reported at lesshint/lesshint#433

  • Node Version: 8.5.0
  • NPM Version: 5.4.2
  • postcss-values-parser Version: 1.3.0

This issue is regarding a problem with:

  • Standard CSS
  • LESS
  • SCSS
  • SASS

If you have a large amount of code to share which demonstrates the problem you're experiencing, please provide a link to your
repository rather than pasting code. Otherwise, please paste relevant short snippets below.

opacity -webkit-filter

Expected Behavior

An AST similar to this:

Word {
  value: 'opacity',
  ...
}
Word {
  value: '-webkit-filter'
  ...
}

Actual Behavior

This AST:

Word {
  value: 'opacity',
  ...
}
Operator {
  value: '-',
  ...
}
Word {
  value: 'webkit-filter',
  ...
}

How can we reproduce the behavior?

const parser = require('postcss-values-parser');
const ast = parser('opacity -webkit-filter', {
    loose: true,
}).parse();

ast.walk((node) => {
    console.log(node);
});

The issue is only present in loose mode. In the regular/strict mode it works as expected. It also seems to work as expected when the prefixed value is the first one, but when it's the second (or more) it starts emitting operator nodes.

@shellscape

This comment has been minimized.

Show comment
Hide comment
@shellscape

shellscape Sep 26, 2017

Owner

This makes me die a little inside 😆 the operators are such a pain in the butt

Owner

shellscape commented Sep 26, 2017

This makes me die a little inside 😆 the operators are such a pain in the butt

@jwilsson

This comment has been minimized.

Show comment
Hide comment
@jwilsson

jwilsson Sep 26, 2017

Collaborator

Oh, btw. I forgot to mention. I think @donotknow might be interested in submitting a fix, right?

Collaborator

jwilsson commented Sep 26, 2017

Oh, btw. I forgot to mention. I think @donotknow might be interested in submitting a fix, right?

@shellscape

This comment has been minimized.

Show comment
Hide comment
@shellscape

shellscape Oct 9, 2017

Owner

@jwilsson would looooove a fix PR

Owner

shellscape commented Oct 9, 2017

@jwilsson would looooove a fix PR

@jwilsson

This comment has been minimized.

Show comment
Hide comment
@jwilsson

jwilsson Oct 15, 2017

Collaborator

@shellscape Hmm, I'll see if I can figure it out 😁

Collaborator

jwilsson commented Oct 15, 2017

@shellscape Hmm, I'll see if I can figure it out 😁

donotknow pushed a commit to donotknow/postcss-values-parser that referenced this issue Feb 17, 2018

donotknow pushed a commit to donotknow/postcss-values-parser that referenced this issue Feb 17, 2018

@donotknow

This comment has been minimized.

Show comment
Hide comment
@donotknow

donotknow Feb 17, 2018

So this seems to be because of the difference between loose/non-loose here:

https://github.com/shellscape/postcss-values-parser/blob/master/lib/parser.js#L177-L187

      if (!this.options.loose) {
        if (this.nextToken[0] === 'word') {
          return this.word();
        }
      }
      else {
        if ((!this.current.nodes.length || (this.current.last && this.current.last.type === 'operator')) && this.nextToken[0] === 'word') {
          return this.word();
        }
      }
    }

@jwilsson @shellscape I read through the associated history (PR #24 over here and lesshint/lesshint#340 over on lesshint) it seems like that was just restored to maintain lesshint’s old functionality. The new (and improved) postcss-values-parser seems to be more correct (+5 is a number, it’s just a positive number; it’s not an operator & number combination).

And then just update on the lesshint’s side to account for that in the spaceAroundOperator which is the thing that should be checking the spaces within that number based on it’s sibling nodes instead of relying on it being an operator.

So! I’m going to create some PRs to explain myself a little, which’ll be easier in code I think.

donotknow commented Feb 17, 2018

So this seems to be because of the difference between loose/non-loose here:

https://github.com/shellscape/postcss-values-parser/blob/master/lib/parser.js#L177-L187

      if (!this.options.loose) {
        if (this.nextToken[0] === 'word') {
          return this.word();
        }
      }
      else {
        if ((!this.current.nodes.length || (this.current.last && this.current.last.type === 'operator')) && this.nextToken[0] === 'word') {
          return this.word();
        }
      }
    }

@jwilsson @shellscape I read through the associated history (PR #24 over here and lesshint/lesshint#340 over on lesshint) it seems like that was just restored to maintain lesshint’s old functionality. The new (and improved) postcss-values-parser seems to be more correct (+5 is a number, it’s just a positive number; it’s not an operator & number combination).

And then just update on the lesshint’s side to account for that in the spaceAroundOperator which is the thing that should be checking the spaces within that number based on it’s sibling nodes instead of relying on it being an operator.

So! I’m going to create some PRs to explain myself a little, which’ll be easier in code I think.

@donotknow donotknow referenced this issue Feb 17, 2018

Open

Browser prefix is operator #45

2 of 2 tasks complete

donotknow added a commit to donotknow/postcss-values-parser that referenced this issue Feb 17, 2018

donotknow added a commit to donotknow/postcss-values-parser that referenced this issue Feb 17, 2018

donotknow added a commit to donotknow/postcss-values-parser that referenced this issue Feb 17, 2018

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