Report better errors for values parsed from a PostCSS stylesheet #102

nex3 · 2020-02-11T03:01:58Z

Breaking Changes?

yes
no

If yes, please describe the breakage.

Please Describe Your Changes

Allow the user to provide a PostCSS node as context for parse(), and use that to generate better error messages in Node.error().

Fixes #98

TODO:

tests
documentation
updating type declaration

Please let me know what the best way is to test something like this.

nex3 · 2020-02-17T21:18:35Z

@shellscape friendly ping!

shellscape

Please let me know what the best way is to test something like this.

You'll want to generate errors that would make use of these changes. We'll also want to see what errors look like without these changes and what the improvements are.

shellscape · 2020-02-17T21:38:19Z

lib/SubInput.js

+
+// A PostCSS Input that exposes a substring of a larger Input as though it were
+// the entire text to be parsed.
+module.exports = class SubInput {


Without seeing how this is used (via tests) I can't comment on whether or not this is a good name for this.

Also, why does this not inherit from Input?

Without seeing how this is used (via tests) I can't comment on whether or not this is a good name for this.

It's not used directly, just instantiated from lib/input.js.

Also, why does this not inherit from Input?

We're not re-using any of Input's methods, we're just matching its API. In static-typing terms, we're implementing Input's interface rather than extending its implementation.

shellscape · 2020-02-17T21:41:00Z

lib/index.js

-    const input = new Input(css, options);
+    let input;
+    if (options.context) {
+      assert(options.lineInContext);


using assert outside of tests is an antipattern imho. When/if that fails, it won't provide the user with any kind of a meaningful error message.

use RangeError and describe in detail what the user/caller is missing.

shellscape · 2020-02-17T21:42:26Z

lib/index.js

+    });
+  },
+
+  parseAtRuleParams(rule) {


why is this here?

never got a reply on this

shellscape · 2020-02-17T21:42:32Z

lib/index.js

    return parser.root;
  },

+  parseDeclValue(decl) {


why is this here?

never got a reply on this

shellscape · 2020-02-17T21:44:11Z

lib/index.js

  parse(css, options) {
-    const input = new Input(css, options);
+    let input;
+    if (options.context) {


options.context isn't a documented option (neither are options.lineInContext nor columnInContext) so I can't comment on them. documentation isn't only useful to end users after merge, it's a key component of new feature PR reviews.

Added documentation, let me know what you think.

nex3

You'll want to generate errors that would make use of these changes. We'll also want to see what errors look like without these changes and what the improvements are.

Can you help me figure out how to do this? I can't find an example of a test that checks the .error() API.

In the meantime, here's a manual example:

const postcss = require('postcss');
const pvp = require('./lib');

function run(value) {
  try {
    value.walkFuncs(function(func) {
      if (func.name === 'var') {
        throw func.error('Undefined variable!');
      }
    });
  } catch (error) {
    console.log(error.toString());
  }
}

const root = postcss.parse(`
p {
  color: rgb(var(--some-color) / 70%);
}`, {from: 'input.css'});

console.log("== Old message");
run(pvp.parse(root.nodes[0].nodes[0].value));

console.log("== New message");
run(pvp.parseDeclValue(root.nodes[0].nodes[0]));

This is what the output looks like:

== Old message
CssSyntaxError: <css input>:1:5: Undefined variable!

> 1 | rgb(var(--some-color) / 70%)
    |     ^

== New message
CssSyntaxError: /home/nweiz/goog/postcss-values-parser/input.css:3:14: Undefined variable!

  1 | 
  2 | p {
> 3 |   color: rgb(var(--some-color) / 70%);
    |              ^
  4 | }

nex3 · 2020-02-17T21:54:25Z

lib/SubInput.js

+
+// A PostCSS Input that exposes a substring of a larger Input as though it were
+// the entire text to be parsed.
+module.exports = class SubInput {


Without seeing how this is used (via tests) I can't comment on whether or not this is a good name for this.

It's not used directly, just instantiated from lib/input.js.

Also, why does this not inherit from Input?

We're not re-using any of Input's methods, we're just matching its API. In static-typing terms, we're implementing Input's interface rather than extending its implementation.

nex3 · 2020-02-17T22:13:06Z

lib/index.js

  parse(css, options) {
-    const input = new Input(css, options);
+    let input;
+    if (options.context) {


Added documentation, let me know what you think.

nex3 · 2020-02-17T22:13:10Z

lib/index.js

-    const input = new Input(css, options);
+    let input;
+    if (options.context) {
+      assert(options.lineInContext);


nex3 · 2020-03-09T18:12:16Z

Friendly ping again!

shellscape · 2020-03-09T18:50:21Z

Any word on the CI failures?

nex3 · 2020-03-09T20:35:09Z

I'm not sure exactly how the tests work, but it looks like they're verifying the internal structures of the parsed nodes, which are intentionally changing. Is there a way I should regenerate the expectations?

nex3 · 2020-03-18T22:28:47Z

Ping again—this is a major sticking point for Google's use of postcss-values-parser. I'd really like to avoid having to move off this package, but if contributions can't be accepted quickly we may be forced to do so.

shellscape · 2020-03-18T22:32:03Z

Heya. I'm currently putting in 60 hour weeks and taking care of our daughter who's home now due to the pandemic. Sorry that I can't move faster for your contribs. If you need to move away from this package no one will blame you, but pressure isn't a motivational right now.

nex3 · 2020-03-18T22:37:06Z

Fair enough—family comes first.

shellscape · 2020-03-23T00:17:36Z

So taking another look at CI, your tests are failing because the snapshots don't match. If you're not familiar with snapshot testing, I'd highly recommend getting up to speed on that, as you'll run across that pretty often in the ecosystem.

Ava's Snapshot Docs https://github.com/avajs/ava/blob/master/docs/04-snapshot-testing.md

Also pervasive, are Jest's snapshots https://jestjs.io/docs/en/snapshot-testing upon which Ava's are loosely based.

shellscape · 2020-03-23T00:30:29Z

Normally I'm loathe to post consecutive replies, but they're addressing different things, so here we go:

I'm still not keen on the overall approach, but I'm willing to think more about it - if you add some tests.

Snapshots are handy here, so I can quickly view the differences between the old output and the output with your changes. I'm also not sure why parseDeclValue and parseAtRuleParams were added as exports. Taking a look at them and the util function they're using, they look like they should be external helpers that utilize the package. But because there aren't any tests on them, I can't visualize quickly how they're intending on being used and if they should be first-class exports. I don't same that to shame, only that the lack of tests is hurting my ability to quickly make a call on the PR. Your comment #102 (review) showed exactly how you should construct a test for that, and you've already managed to identify how to capture the error content. All you need do is create a snapshot from that.

And of course most importantly, tests to show this parsing a value in a file, and passing the context. The test/fixtures directory is handy for keeping files around for tests.

Closes shellscape#98

nex3 · 2020-03-31T00:58:04Z

Thanks for the tips! I've fixed the existing test failures and added new tests for error-handling in the context of a larger PostCSS document.

I really thought I'd written a reply about parseDeclValue and parseAtRuleParams; sorry about that! 😅 They're utility functions for the cases where the CSS value you're parsing is either a declaration's value or a parameter for an at-rule. I think these first-class utility functions are warranted for of a combination of two reasons:

These are by far the most common sources of CSS values that are parsed by this package.
Properly configuring the lineInContext and columnInContext options for these two options is a little tricky, since PostCSS doesn't automatically provide the source line/column for the start of the values (only the beginning of the entire declaration or at-rule).

Utility functions makes it much more likely that users will correctly configure parsing for these values, which in turn will mean that their error messages will be clearer and more accurate.

Update: It looks like the test are still failing on CI, but with a stack trace that's entirely outside this package. It doesn't reproduce locally for me. I'm not sure what to do about that.

shellscape · 2020-04-17T15:27:22Z

This project is on my list for open source today. Are you still waiting to use this functionality or have you or your team moved on to use an alternative?

nex3 · 2020-04-17T17:01:56Z

This is definitely high priority for us!

nex3 changed the title ~~This PR contains:~~ This PR contains: Report better errors for values parsed from a PostCSS stylesheet Feb 11, 2020

nex3 requested a review from shellscape February 11, 2020 23:27

shellscape requested changes Feb 17, 2020

View reviewed changes

nex3 commented Feb 17, 2020

View reviewed changes

nex3 requested a review from shellscape March 9, 2020 18:12

shellscape changed the title ~~This PR contains: Report better errors for values parsed from a PostCSS stylesheet~~ Report better errors for values parsed from a PostCSS stylesheet Mar 23, 2020

nex3 added 10 commits March 30, 2020 16:34

Regenerate snapshots from a clean master

9e6cc98

Report better errors for values parsed from a PostCSS stylesheet

1698e19

Closes shellscape#98

Add documentation

7de90f1

Don't use assert() for errors

2c86e26

Update typings

e0c32ae

Fix bugs

b8b374a

Regenerate snapshots

dfc8324

Add a dependency and regenerate Ava headers

0f73974

Add tests

9e5e985

Fix lints

a7ba2b6

nex3 force-pushed the errors-with-context branch from e35ed32 to a7ba2b6 Compare March 31, 2020 00:39

shellscape closed this Apr 17, 2020

Uh oh!

Report better errors for values parsed from a PostCSS stylesheet #102

Report better errors for values parsed from a PostCSS stylesheet #102

Uh oh!

Conversation

nex3 commented Feb 11, 2020 • edited by shellscape Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Changes?

Please Describe Your Changes

Uh oh!

nex3 commented Feb 17, 2020

Uh oh!

shellscape left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nex3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nex3 commented Mar 9, 2020

Uh oh!

shellscape commented Mar 9, 2020

Uh oh!

nex3 commented Mar 9, 2020

Uh oh!

nex3 commented Mar 18, 2020

Uh oh!

shellscape commented Mar 18, 2020

Uh oh!

nex3 commented Mar 18, 2020

Uh oh!

shellscape commented Mar 23, 2020

Uh oh!

shellscape commented Mar 23, 2020

Uh oh!

nex3 commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shellscape commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nex3 commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nex3 commented Feb 11, 2020 •

edited by shellscape

Loading

nex3 left a comment •

edited

Loading

nex3 commented Mar 31, 2020 •

edited

Loading

shellscape commented Apr 17, 2020 •

edited

Loading