Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

error / line reporting #6

Closed
tj opened this Issue · 14 comments

5 participants

@tj
Owner
tj commented

tough to track down errors

@jonathanong

there's position now, but we should document how plugin authors should throw errors. err.position = rule.position should be enough. what say you?

@andreypopp

+1 on this, currently most of errors are cryptic...

Also rework should catch those errors and format them accordingly using position prop.

@ianstormtaylor

+1 on this, was just thinking about how jank the errors are. how do we want to go about this? im happy to contribute brain cycles and LOC :)

@jonathanong

reworkcss/rework-inherit#7 for reference as well

@andreypopp

I think this is more of an issue for rework plugins.

@jonathanong

still need to add it to the readme.

tj wanted to print out the context around where the error occurred, but i'm too lazy to implement it. maybe we can just add it to the readme or wiki or something and call it a day.

@ianstormtaylor

what do you think the error object should have? i was thinking that we'd have a Rework#error method that would take (declaration, message) and return a nice error with:

{
  message: message + ' near line ' + line + ':' + column,
  start: {}, // start object straight from css-parse
  end: {}, // end object straight from css-parse
  source: '', // the line in the source for the declaration
  context: {
    start: '', // two lines from before `source` line
    end: '' // two lines after the `source` line
  }
}

open to ideas. will try to implement now

@andreypopp

I think if a plugin author would just add a position information (from css-parse which includes filename, line and column) to an error object before throwing it, that would be sufficient.

Then rework or any other tool build on top of it will format error message accordingly to include context if needed. I actually prefer for rework itself not to do that because formatting error message with context will involve I/O (for reading source file) and that's a thing better to avoid, I think.

@jonathanong

well, no. you can just save the source string like @ianstormtaylor did and recreate the error without any I/O.

@andreypopp

@jonathanong this in case of rework usage when it gets a CSS string, but what if I use something like rework-npm which imporrts CSS from other files? #133 doesn't support that case as I understand.

This is why I'm for a solution which just requires plugin to provide .position property on thrown errors.

@jonathanong

yeah i'm thinking it should support both.

@andreypopp

Also #133 further couples rework plugins with rework API — previously plugins didn't need to call Rework methods, just read configuration options. I don't understand why Rework#error is superior to just having .position property on error objects.

@andreypopp andreypopp referenced this issue from a commit in andreypopp/rework
@andreypopp andreypopp Better error reporting
Fixes #6 and is an alternative approach as taken in #133.

This catches an error in `.use(...)` and format it using
`Rework.prototype.formatError(...)` which could be overriden by supplying
`options.formatError(...)` function.
9a28fa5
@andreypopp andreypopp referenced this issue from a commit in andreypopp/rework
@andreypopp andreypopp Better error reporting
Fixes #6 and is an alternative approach as taken in #133.

This catches an error in `.use(...)` and format it using
`Rework.prototype.formatError(...)`.
1cad51c
@andreypopp andreypopp referenced this issue from a commit in andreypopp/rework
@andreypopp andreypopp Better error reporting
Fixes #6 and is an alternative approach as taken in #133.

This catches an error in `.use(...)` and format it using
`Rework.prototype.formatError(...)`.
a9a27b7
@andreypopp andreypopp referenced this issue from a commit in andreypopp/rework
@andreypopp andreypopp Better error reporting
Fixes #6 and is an alternative approach as taken in #133.

This catches an error in `.use(...)` and format it using
`Rework.prototype.formatError(...)`.
146791d
@jonathanong

okay we've agree on err.position =. i'll leave this open as a "we need to document this" issue.

@lydell
Owner

This is documented now.

@lydell lydell closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.