Fix some bugs with less

[iamkrillin]

Changed where the Comp Chainer wont run when a comp fails
Changed srv-less to not use the short hand method
-> this fixed Unexpcted token U
-> this also fixed comp errors with less when the file was empty
Changed server base to detect and stop trying when the node process
fails to start
Changed server base to not keep trying if the resource takes forever to
start up

[madskristensen]

Thanks!

@a11 Wanna take a quick look at this?

[am11]

@iamkrillin, it is looking good. Out of curiosity, besides the .then() promise pattern (which is probably better than the traditional cb approach), was there anything broken in current implementation of srv-less.js? I am asking because less.js v2 introduced some breaking changes and I think all existing tests were passing when I upgraded to comply with v2 API changes. Please make sure the tests are still passing (Tip: you can rerun the failing tests if VS-Experimental-Host crashes while running the tests, the objective is to make sure if the test failed due to the syntax or behavioral error. Just ignore those sweetjs related tests failing)
Also, 👍 for rest of changes in .cs files, I hope it will relax all the issues with fuzzy (or no) reproduction steps.

Separately, I think we should also update the node-sass service, since there are couple of breaking changes introduced in v2 (beta) there as well: https://github.com/sass/node-sass/releases/tag/v2.0.0-beta (and few more to come in next beta). When WE PreBuild script (msbuild task) runs, it takes the edge version of packages so that might give a blowback to node-sass users at this point, if it is shipped without complying with those changes.

[iamkrillin]

@am11 No, nothing was broken however changing it to use a promise fixed "Unexpected Token U"
#1696

The only test that is broken for me is "SelectorExpansionTest" but its breaking in the ContentTypeManager initializer. So I'm not sure what to make of that. Perhaps I don't have something installed.

I changed the prebuild task to use sass 1.1.4 for now, I figured that was a easy enough fix until the new API could be integrated properly

[am11]

@iamkrillin, cool!

We can debug SelectorExpansionTest later. Probably, (and hopefully) it is a line ending or whitespace.

About node-sass v2, while I am laying lazy atm to make a proper patch, would to like to take a stab at it? here is a pseudo diff for srv-scss.js:

L3-4:
- path = require("path"), 
- xRegex = require("xregexp").XRegExp; 
+  path = require("path");

L17-18: 
- success: function(css, map) {
-  map = JSON.parse(map);
+ success: function(result) {
+  var css = result.css;
+  var map = result.map; // TODO: map is a parsed JS object at present, 
                                        // it will be changed to string in next release, 
                                       // which means the JSON.parse will be back

Finally, srv-scss.js#L87-L104, will become:

error: function(error, code) { 
  writer.write(JSON.stringify({ 
    Success: false, 
    SourceFileName: params.sourceFileName, 
    TargetFileName: params.targetFileName, 
    MapFileName: params.mapFileName, 
    Remarks: "SASS: An error has occured while processing your request.", 
    Details:  error.message, 
    Errors: [{ 
      Line: error.line, 
      Column: error.column, 
      Message: "SASS: (" + code + ")" + error.message, 
      FileName: regex.fileName, 
      FullMessage: "SASS: (" + code + ")" + error.fullMessage 
    }] 
  })); 
  writer.end(); 
} 

[iamkrillin]

@am11 I updated the srv-scss to comply with the new API per the documentation, I found no mention of the code argument in the error handler, course I felt that the documentation there was rather sparse.

On another note, the prebuild task started to fail on me with the following error:
"There was a problem validating the file "Resources\nodejs\tools**". Illegal characters in path."

Course that prebuild task has always been on the flaky side for me.

[am11]

@iamkrillin, going by the readme example, https://github.com/sass/node-sass/#examples error is an object which has a member code.
Also, you removed .line (wonder why?) and didn't added .column (which is new in v2) I agree that I forgot to add .line and .column in docs.

I felt that the documentation there was rather sparse.

PRs are most welcome on node-sass repo! :)

Nonetheless, you can always play around with it quickly in CLI, for instance:

# powershell
mkdir /temp; cd /temp
npm install node-sass
notepad /temp/blah.scss
# and paste this for example:   pre:first-of-type { z-index: 12; 
# note I deliberately dropped the closing parenthesis
node
# you have entered node interactive console

require("node-sass").render({
  file: "/temp/blah.scss",
  success: function(result) { console.warn(result)},
  error: function(error){console.error(error)}})

# It will print:

> { status: 1,
  file: '/temp/blah.scss',
  line: 1,
  column: 32,
  message: 'invalid property name',
  code: 1 }

So you can ignore status member of error, but rest of them can be casted into WE's error format.

[am11]

Regarding prebuild, if you haven't changed anything in its .cs and project's .csproj file, then it should work fine.
Dealing with prebuild errors is painful. Sometimes, it is due to npm dependencies and sub-dependencies. But lets try the old fashioned way:

• format and reinstall windows .. just kidding :)
• clean solution (Build > Clean Solution)
• remove EditorExtension/bin and obj/
• remove packages in solution directory.
• remove EditorExtension/Resources/nodejs

And rebuild!

[am11]

I have updated the node-sass README: sass/node-sass@8075bd6.

[iamkrillin]

@am11 I followed the directions you provided to resolve my build issue and it didn't help. Turns out the problem was that after all the node stuff installed some of the file paths were to long for the build in classes to handle, so the build bombed out during the packaging.

I noticed that the packager was excluding certain files from the vsix to save space on the deployable, I moved (most) of this logic into the prebuild task so that I could make use the the LongPath handlers, this allowed me to remove the test files and so forth.

Note: This did result in a larger deployable (feel free to tweak more). I figured a more reliable build process and thus a more reliable deployment process was worth the cost.

After i done that, I further modified the the prebuild task to clear out all the node files before it tried to add modules. This resulted in a 100% reliable build for me

The last change I made here was if the a node module throws an exception, the message is now printed to the console.

After I resolved all the build issues I done some more testing with the SASS handler and it is now working as expected.

[iamkrillin]

@am11 Not sure what you meant there. I removed the JSON.parse() line cause it was breaking the handler completely.

[am11]

Great work @iamkrillin! 👍

Just to be sure, you know that we are black-listing and white-listing the packages files in csproj file here, right? If this solves the long path issue (something I described here), which MSBuild task can't handle OOTB, then its absolutely perfect!

Would be really nice if @SLaks can provide some feedback / further pointers on the PreBuild changes..

---

One more thing, perhaps you would like to do a separate PR for this; please change the npm package URL to https://github.com/npm/npm/releases/latest ? According to npm/npm#6474, npm is discontinued from node-distribution website (not sure why they did that, after all npm is their chief package manager!)

So, to capture the actual URL to zip file on latest release page, you would probably need to capture the redirected URL (followed by https://github.com/npm/npm/releases/latest) and interpolate the string using something like http://stackoverflow.com/a/26088551 (translate PHP to C#). :)

Thanks.

[iamkrillin]

@am11 I figured as much, but the problem I was experiencing that method was still breaking the build due to long paths. Perhaps that was just as "My Machine" type of problem but moving that out of the proj file into the prebuild task fixed it.

[am11]

With npm v3, the long path issue for Windows would be fixed (meaning npm dedup would asset maximum effort to flatten the dependencies). This 256 chars limit is an ordeal. They should reconsider it in vNext of OS.

I remember @madskristensen was having this issue once and copying the solution to drive's root fixed it. (but it didn't worked for me last time due to node-sass' long dependency chain, which is quite reduced since).

You can share (failing) build logs via http://gist.github.com/. Not sure if we can point out something, but you never know.

[iamkrillin]

@am11 for my clarification, is there something else you were wanted me to do?

[am11]

@iamkrillin nope. LGTM!
Lot of folks were having issues with less compilation. So again thank you for fixing it and yay for promise pattern!. 😄 👍

Keep them coming! If you need to talk about some code in WE, you can tag me here. Also, I am mostly available on IRC #Node.js and #libuv @freenode with same nick.

//cc @madskristensen

[madskristensen]

Thanks!! :)