-
-
Notifications
You must be signed in to change notification settings - Fork 32
Report better errors for values parsed from a PostCSS stylesheet #102
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
Conversation
@shellscape friendly ping!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/index.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/index.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never got a reply on this
lib/index.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never got a reply on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation, let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation, let me know what you think.
lib/index.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Friendly ping again!
Any word on the CI failures?
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?
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.
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.
Fair enough—family comes first.
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.
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.
e35ed32 to
a7ba2b6
Compare
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
lineInContextandcolumnInContextoptions 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.
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?
This is definitely high priority for us!
Uh oh!
There was an error while loading. Please reload this page.
Breaking Changes?
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 inNode.error().Fixes #98
TODO:
Please let me know what the best way is to test something like this.