Catch rule execution exceptions

[ryprice]

This try/catch isolates exceptions from a single rule, such as #5003 #4865, to identify what rule failed and prevent the build step from crashing.

npx stylelint example

[jeddy3]

@ryprice Thanks for the pull request. This approach is in line with how we catch parse errors from the selector parser.

Anyone else have thoughts on this pull request?

[ybiquitous]

In this approach, it seems that a stack trace of a thrown error is dismissed (we might not be able to know the error details).

[ryprice]

@jeddy3 should the wrapper be generalized? Are there a bunch of different classes of rules that should all be wrapped in the same way?

@ybiquitous You're right about that, but it gives the caller enough info to know what rule failed and what code caused the failure. Is there a way to output extra info in a verbose mode? Do you think isolating errors without failing the build go against the design of stylelint?

Anyway, for my situation, the most important part of this fix is indicating what file and rule failed before crashing. That was missing before, and it made unblocking myself an investigation into stylelint rather than some quick tinkering with the offending sass file.

[ybiquitous]

In the structured output mode like --formatter=json, we can add extra details like a stack trace.

But, in the one-line output mode like --formatter=compact, multi-line text like a stack trace may be dismissed, and so I'm worried that users may be hard to know the cause of errors. 🤔

the most important part of this fix is indicating what file and rule failed before crashing

Surely, the error message should be improved.
I'm not sure whether it's best or not, but the following may be a solution:

try {
  // ...
} catch (err) {
  console.error(err) // show original stack trace
  throw new Error(`${err.message}: file=${file}, rule=${rule}`)
}

[m-allanson]

I agree that losing the stacktrace makes it harder to debug problems.

It looks like stylelint is 'crashing appropriately' and erroring out when it hits a problem in a rule. But the crash information isn't clearly surfaced when running as part of a webpack build.

@ryprice does that sound right? If that's the case then maybe the stylelint webpack plugin could be updated to output more helpful error information?

[ryprice]

@m-allanson I agree that the stylelint-webpack-plugin should have been more graceful as well. I think there's still a stylelint fix here to explain the failure in plain english, because running stylelint via npx also throws the stacktrace without any explaination. Maybe something similar to what @ybiquitous suggested, but still throw the original exception?

try {
  // ...
} catch (err) {
  console.error(`Unexpected exception running Stylelint rule '${rule}' on '${fileName}'. Message: ${err.message}`);
  throw err;
}

[ybiquitous]

@ryprice Surely, it is better to throw the original exception in order to keep the original stack trace. 👍

Another way is to add a new property to a caught Error object as follows:

try {
  // ...
} catch (err) {
  err.ruleDetails = {
    ruleName: ruleName,
    filePath: postcssResult.opts ? postcssResult.opts.from : undefined,
  };
  throw err;
}

Like configurationError() adds the code property:

stylelint/lib/utils/configurationError.js

Lines 9 to 14 in 5a84657

	/** @type {Error & {code?: number}} */
	const err = new Error(text);
	err.code = 78;
	return err;

We can control an output error message in cli.js as follows:

if (err.ruleDetails) {
  process.stderr.write(`Unexpected error occurs: rule=${err.ruleDetails.ruleName}, file=${err.ruleDetails.filePath}` + EOL);
}
process.stderr.write(err.stack + EOL);

stylelint/lib/cli.js

Lines 584 to 589 in 5a84657

	function handleError(err) {
	process.stderr.write(err.stack + EOL);
	const exitCode = typeof err.code === 'number' ? err.code : 1;
	process.exitCode = exitCode;
	}

[m-allanson]

@ryprice I like your approach of throwing the original exception in addition to the helpful error message. 👍

@ybiquitous also has a good take on this, as his approach could be adapted to handle errors from any part of stylelint.

Either way works for me!

[ryprice]

Adding ruleName and filePath to the error object is interesting, but I feel like it's probably a solution that requires more thought to implement wholesale for all rule processing and as a contract between stylelint and stylelint-webpack-plugin. For now, I'll just spit it out as a console.error and throw the original exception. Then in stylelint-webpack-plugin, I'll at least put a bit more detail to indicate the error came from stylelint.

[ryprice]

How do we feel about disabling this rule here?

[ybiquitous]

IMO, disabling it here seems no problems. 👍
The current ESLint configuration may be a bit too strict, so it seems good to me to loosen it via the allow option as follows:

{
  "no-console": ["error", { "allow": ["warn", "error"] }]
}

https://github.com/stylelint/eslint-config-stylelint/blob/aecb506259e5371e162ee590b67d671d1bc72095/.eslintrc.js#L33

See also https://eslint.org/docs/rules/no-console#options

[ybiquitous]

About this, I've opened a new issue: stylelint/eslint-config-stylelint#108

[ybiquitous]

When I test this change with --stdin, the message includes undefined instead of unknown file:

$ cat a.sass | bin/stylelint.js --stdin --syntax=sass
Rule indentation threw an unknown exception from undefined

So, can we add test cases about this change? It seems good to me to add them to lib/__tests__/standalone.test.js.

[ybiquitous]

[IMO] I have some suggestions about this message:

• How about quoting a rule name?
• How about quoting a file name?
• How about using <input css> if a file name is not present?

For example:

Rule "indentation" threw an unknown exception from "/home/foo/button.sass"

Rule "indentation" threw an unknown exception from <input css>

See also:

stylelint/lib/createPartialStylelintResult.js

Line 81 in f0b5aa8

source: cssSyntaxError.file || '<input css 1>',

[jeddy3]

Closing stale issues.