Skip to content

Updated walk() to print out more information on exception#1093

Closed
nikhilgaba wants to merge 1 commit intopostcss:masterfrom
nikhilgaba:containerErrorImprovement
Closed

Updated walk() to print out more information on exception#1093
nikhilgaba wants to merge 1 commit intopostcss:masterfrom
nikhilgaba:containerErrorImprovement

Conversation

@nikhilgaba
Copy link
Copy Markdown
Contributor

This is a modification to the walk() method found for the Container object. The difference from the original is that it now attempts to print out more information to help a developer locate where the problem occurred.

@ai
Copy link
Copy Markdown
Member

ai commented Dec 8, 2017

@nikhilgaba It is an interesting idea. But we have 5 problems:

  1. We need to copy error class.
  2. Maybe instead of changing message, we should change .stack. Because we are not sure how the error will be caught. Maybe there is some error.message === someText above.
  3. We need an option to disable it.
  4. We can enable it by default only in a major release.
  5. Error must be captured not only in walk, but also in postcss.plugin and in Promise if plugin return it to postcss.plugin.

If you are ready to pass this way, I will help you on the way :).

@ai
Copy link
Copy Markdown
Member

ai commented Dec 8, 2017

How I see a process of adding it to PostCSS:

  1. We change a way to add this data to error (class coping, replacing stack, adding extra properties to error object)
  2. We add this magic to postcss.plugin.
  3. We add an option and disable it by default.
  4. We release it in 6.1. It will be switched off by default.
  5. We will ask postcss-loader, gulp-postcss and postcss-cli to turn the option on in their next major release.
  6. In 7.0 we will switch it on by default.

result = child.walk(callback);
}
return result;
} catch (exception) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exception => err

// If an error occurs, attempt to print
// out information that will narrow down
// where the problem can be found.
throw new Error(
Copy link
Copy Markdown

@michael-ciniawsky michael-ciniawsky Dec 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a new Error Type (Instance) instead please ? So third party implementations can handle it correctly via e.g if (err.name === 'WalkerError') handler... equal to how it works for err.name === 'CssSyntaxError' already

// But not WalkerError please, it's a really bad name :)
class WalkerError extends Error {
   constructor (err) {
    super()
   
    this.name = 'WalkerError'
    this.message = ... // handle/format message (file, message, code frame etc..)
    this.stack = ... // handle/format stack trace

    Error.captureStackTrace(this, this.constructor)
  }
}
catch (err) {
  throw new WalkerError(err)
}

@ai
Copy link
Copy Markdown
Member

ai commented Dec 8, 2017

  1. Error must be captured not only in walk, but also in postcss.plugin and in Promise if plugin return it to postcss.plugin.

Forget it. I was wrong in this request (there is no way to detect source node or of walk).

@ai
Copy link
Copy Markdown
Member

ai commented Dec 8, 2017

I suggest:

} catch (exception) {
  if (result.opts.errorSource) {
    exception.postcssNode = child
    exception.stack = "…"
  }
  throw exception
}

@michael-ciniawsky
Copy link
Copy Markdown

michael-ciniawsky commented Dec 8, 2017

☝️ How will the thrown {Error} look like then ? How to catch && handle it correctly ?

postcss(plugins).process(css, options)
  .then((result) => result)
  .catch((err) => {
     // for e.g
     err.name === '?' 
       ? new PostCSSError(err) // Syntax Error, Plugin Error, API Error etc...
       : new Error(err) // Normal (JS) Error 
  })

@ai
Copy link
Copy Markdown
Member

ai commented Dec 17, 2017

@nikhilgaba what do you think about my plan?

@nikhilgaba
Copy link
Copy Markdown
Contributor Author

@ai Plan seems quite sound. I just haven't had a chance to look into the implementation.

@ai
Copy link
Copy Markdown
Member

ai commented Dec 20, 2017

Don't worry, I will use power of our Cult of Martians to improve your PR.

@ai ai added this to the 7.0 milestone Mar 17, 2018
Copy link
Copy Markdown

@nzec nzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay!

@ai
Copy link
Copy Markdown
Member

ai commented Jul 16, 2018

I merged and fixed it manually b2530e0

@ai ai closed this Jul 16, 2018
@ai
Copy link
Copy Markdown
Member

ai commented Jul 16, 2018

Released in 7.0

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants