Refactor LessError and lesscHelper.formatError

[kirillrogovoy]

Objective (what was wrong)

This fixes my initial concern of this issue: #2951

Solution

Firstly, I want to drop a few facts about Errors in Javascript.

1. It has 3 standard fields: name, message, stack and might have any number of additional, user-defined fields. Generally, such fields are designed to keep different pieces of information related to the error.
2. It has it's own .toString() method which overrides default Object.prototype.toString(). This method is designed to create an as detailed message for the developer as possible using those informational fields.
3. Error might have user-defined methods to create specific sets of information out of informational fields for sophisticated cases. Also, these could be just pure functions instead of prototype-bound methods, it's conceptually the same thing.

Then, we have 3 possible usages of Less:

1. lessc - the CLI way
2. Browser
3. Direct call of less.render from your custom code.

And then, we had a helper function called lesscHelper.formatError which accepted an instance of LessError and produced a human-readable message with optional support of shell colors.

The problem was this method was only used in lessc and in the test launcher, when comparing the actual error message to the excepted one. So, this function was never called when calling less.render directly resulting in unavailability of getting a detailed user-friendly message as if you use lessc.

After some analysis I understood that, according to facts I described above, lesscHelper.formatError is actually what .toString should do. Thus, .toString can be used in lessc, in tests and, obviously, it's automatically called when Node tries to display the error in your console, resolving the issue I faced.

I didn't touch anything regarding the browser as it was out of the scope of my problem.

Critique is welcomed!

[kirillrogovoy]

Any progress on this, guys?

@matthew-dean

[matthew-dean]

@kirillrogovoy Sorry, been busy in the last month, so I didn't spot this. I'll review as soon as I can. Thanks!

[matthew-dean]

@kirillrogovoy I notice that although you moved less-node error handling into the core less-error.js file, you didn't actually refactor less-browser to use that error formatting, and it contains duplicate code. (I discovered this while writing tests where I needed to omit line / column reporting for some files, and I had to make those changes in two places.)

Is it possible to align less-browser to what less-node is doing, at least for console errors?

Also, less-browser contains two forms of error output: HTML and console. Ideally, "styling" the error would be separate from the generated parts of the error. Meaning: the error output is always identical, but then gets formatted for the Terminal (colors), or the browser console, or as HTML. (Although, I've proposed that HTML formatting of errors could probably be deprecated, since all major browsers now have good debuggers with console output.)

[kirillrogovoy]

@matthew-dean You're right about the code duplication. I didn't touch that browser-related code on purpose - I was to make an initial step by encapsulating LessError "stringification" at the right place, so that place can be used elsewhere. Browser-related code, as I thought, was too much to take at that moment. Anyway, I didn't make any worse since that duplication existed before my change.

I think refactoring of other consumers of LessError is the next step here.

Also, note that LessError knows nothing about CLI output formatting. It only knows which colors should be used, however the options.stylize function comes from the outside.
It's not a perfect decision for LessError still contains the information about the colors which looks a bit weird, but I have an idea to change the stylize interface to take not modifiers (like 'grey' or 'bold') as the second argument, but a name of a place (e.g. 'line', 'columns', 'message', etc.). Thus, the information about the colors would be stored at a particular version of the stylize function. The good thing is that such refactoring could be made at any time.

If I get to refactoring less-browser any soon, I'd try this idea along the way. :)