Skip to content

Refactor LessError and lesscHelper.formatError#2975

Merged
matthew-dean merged 2 commits intoless:3.xfrom
kirillrogovoy:refactor-less-error
Dec 21, 2016
Merged

Refactor LessError and lesscHelper.formatError#2975
matthew-dean merged 2 commits intoless:3.xfrom
kirillrogovoy:refactor-less-error

Conversation

@kirillrogovoy
Copy link
Copy Markdown
Contributor

@kirillrogovoy kirillrogovoy commented Oct 12, 2016

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 kirillrogovoy mentioned this pull request Oct 12, 2016
Closed
@kirillrogovoy
Copy link
Copy Markdown
Contributor Author

kirillrogovoy commented Oct 28, 2016

Any progress on this, guys?

@matthew-dean

@matthew-dean
Copy link
Copy Markdown
Member

matthew-dean commented Nov 7, 2016

@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 matthew-dean merged commit bf34b95 into less:3.x Dec 21, 2016
@matthew-dean
Copy link
Copy Markdown
Member

matthew-dean commented Jan 2, 2017

@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
Copy link
Copy Markdown
Contributor Author

kirillrogovoy commented Jan 3, 2017

@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. :)

@kirillrogovoy kirillrogovoy deleted the refactor-less-error branch January 3, 2017 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kirillrogovoy @matthew-dean