Fix for issue #592, which hides the original error message with one generated by the error handling logic. #703

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@paulogaspar7
Contributor

I Alexis,

This is the pull request to fix issue #592, as explained here:

Repeating, since this is actually the best place for it:

I did try with compiling my stuff with 1.2.2 but it still has the same problem.

I finally diagnosed the problem and got to a solution I find acceptable. It is a problem related with scopes and timing.

The callback you pass when you call less.Parser.importer() at the top of "parser.js" is at the top importing files Parser scope. However, I think this callback only gets called and registers the content data (the actual less code text) ater an import is parsed. This might be to late for some parsing errors.

Anyway, when there is a parsing error - because there is actually an error on the Less source code which trigger this problem - the code handling the error does not have access to part of the data it counts on to perform its task.

As parte of the error handling, Less tries to display the offending code. It uses the getInput() function (at parse.js) to get that offending code. When it gets an undefined instead of a string of Less code and tries to cut a bit of it to display it, it cause an exception to be thrown. That is the origin of the weird error message reported at issue #592.

This is also why the error is intermittent. The problem at the error handling code is simply hiding the error message for the real problem. Eventually, the programmer fixes the original problem at his Less code and everything works again.

Therefore I tried to load the cache as soon as the less stylesheet code is loaded. Then, of course, it is necessary to pass the data to the top importing parser cache, which requires some reference passing.

I found a solution simple enough for me to feel confortable. Not beautiful, but I don't feel confortable making a big change on your code. Had to keep it as simple as possible.

I tested everything by:

  • Running the tests via "make test";
  • Compiling via "lessc" the Less stylesheets for the current (straight rom the master) Twitter Bootstrap and from the app I am working on;
  • Running Bootstrap with dynamic loading too.

Everything worked fine except for an unrelated issue, present also on 1.2.2 - resolving variables only on dynamic loading on url()s, like this one:

I also removed the use of basename() since it was vulnerable to name clashes - e.g.:

  • when multiple libraries have a "variables.less" file;
  • and there is a single "main.less" stylesheet importing from all libraries.

In my use cases I detected no problems about that either and I was careful to ensure all bits and pieces received as filename the name actually used by the loading logic (for both browser and node).

Please review my changes. Do not hesitate to reach me for any doubts, even via Twitter (same id as here).

Cheers

@lvh
lvh commented May 16, 2012

Apparently this replaced #653 (comment).

Both of these fix the issue for me.

Is there a way I can aid this getting merged?

@rainboxx

@cloudhead Why is this not pulled? People have issues (see: #592), which this pull requests solves. Thanks!

@lukeapage
Member

I'm going to look at this and see if I can get my head round it, hopefully will address it soon..

But.. I don't like the TODO's - I'd rather remove the code, but I want to be really careful you aren't removing functionality anyone else uses.

Also it would be really nice to see some tests around this if we can.

Don't worry neither of the above will stop my pulling, I just need to sort them out myself if you don't.

@paulogaspar7
Contributor

Hi @agatronic, thanks for the attention.

I did quite some testing at the time (this patch is aging) with a complex import tree which included Twitter Bootstrap and causing multiple types of errors across that tree - e.g.: commenting out (used) variable declarations.

Everything worked, which means that:

  • when an error was introduced the original error messages were displayed, instead of being replaced by the "charAt" error message;
  • when there was no error everything worked correctly.

I remember I also had some duplicated style sheet names, like a "style.less" which imported some other file (say "other.less") a directory down, which imported another "style.less" sheet another directory down. I also had that working correctly.

The TODOs are there for whomever processes this patch... which means you.

Can I assist you any further? You can ping me on twitter (same handle) for a faster answer.

@lukeapage
Member

have merged this in locally and added error test cases. Have also sorted out the TODOs. Couldn't create any test cases that reproduce the error, but will continue some work on errors and push this as part of that work.

@lukeapage
Member

pulled

@lukeapage lukeapage closed this Aug 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment