Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Sass template processing #439

Merged
merged 2 commits into from
May 2, 2017
Merged

Adds Sass template processing #439

merged 2 commits into from
May 2, 2017

Conversation

phil-scott-78
Copy link
Contributor

This more or less copies the LESS module but adds Sass processing using LibSass.Net. LibSass.Net is a wrapper around the C library LibSass which worried me a bit, but everything seems to wire up just fine.

The test project is pretty simplistic. I would have liked to have a few test cases for testing out that I got the include functionality right, but I couldn't wrap my head around how to do that easily. But basic verification of a successful call and a failing call are there for now.

I originally started out making this its own separate Nuget package, but I figured with LESS being built in it would only be a matter of time until someone else wanted Sass support and having it out of the box probably makes more sense in terms of growing the user base via out of the box features. I could easily rip it back out if you want to go the seperate package route.

@daveaglick
Copy link
Member

Same deal here - really happy to finally get Sass, but just haven't had a chance to review yet. Will soon.

@daveaglick
Copy link
Member

The code looks great.

I do have some concerns about LibSass.Net though. Specifically:

  • The packages look like they're still marked as pre-release in NuGet and have been for a while. NuGet enforces transitive pre-release flags so at a minimum, this module library will also need to be marked as pre-release on NuGet. I've been hoping to avoid that when possible going forward.
  • The requirement for the Visual C++ library concerns me, especially when thinking ahead to .NET Core. I've been trying to make sure that all future enhancement wouldn't cause problems with a Core port since that's around the corner. I'm not sure LibSass.Net would be easy to port to Core and it doesn't look like there's much activity in that direction on the repo.

That said, I don't know that there are any good alternatives. One thought I had been tossing around earlier was to use Jint (which supports netstandard) to run sass.js, which is a pure JS version of libsass (cross-compiled via emscripten). That would be interesting because it may also open up the door to other Jint-powered libraries for things like Handlebars, better Less support, etc.

Thoughts?

@phil-scott-78
Copy link
Contributor Author

I like it. Let me get a POC running on this branch to kick the tires. Haven't seen Jint before but it looks like a heck of a better option than the MsieJsEngine and requiring C++.

@phil-scott-78
Copy link
Contributor Author

Jint seems to have a major blocker - regex support. Part of its transpiling magic it converts the calls to use the .Net implementation but there's limited code in converting the actual regex string to something .Net likes. I tried converting a few statements but quickly realized I was just randomly smashing combinations of \ [ and ] hoping it would work. Surely not a scalable long term solution.

I'm going to try out VroomJS which is a wrapper around V8. I think the wrapper route is going to be a necessary evil here, especially with linting tools and the such which are so regex heavy. ReactJs.Net uses it and it is cross platform but not in a plug and play kind of way. See their docs here on what's required - https://reactjs.net/guides/mono.html

@phil-scott-78
Copy link
Contributor Author

Sigh, playing around with this today lead to one unfortunate and inevitable conclusion - Wyam.Core getting JavaScriptEngineSwitcher (maybe a step further and use a standard JsPool too) and exposing a IJsEngine for all extensions to share. The more I think about it the more sense that makes.

What I'm not sure about it how to make it cross platform. There's tons of implementations of IJsEngine that seemingly cover the majority of scenarios, the question that I just don't have the experience with it designing it so that they all play nice for both end-users and developers.

@daveaglick
Copy link
Member

You know, I'm okay with exposing a common JS framework and supporting interfaces from Wyam.Core. Given how fundamental JS is to web development and how many valuable libraries are out there with JS implementations, I think it makes a lot of sense.

The interfaces should go in Wyam.Common and probably get exposed from the IExecutionContext. That way any module that needs to can access them. The actual dependencies and implementations should go in Wyam.Core.

You're right, the challenge here is going to be figuring out how to implement so that it's stable cross platform and supports netstandard. I'll try to carve some time tomorrow to look into Jint and others. I've been down this path before and I seem to remember there being some additional alternatives like Jurassic.

@phil-scott-78
Copy link
Contributor Author

Right on. I'm going to add a new test to the highlighting PR that forces the highlight.js auto highlighting mode on. That seems to be the stress test here. It more or less goes through each language's syntax looking for a match using regular expressions. That's where the JS -> IL libraries seem to fall short. I suspect only V8 or Chakra will pass that kind of test.

@daveaglick
Copy link
Member

I'm going to add a new test to the highlighting PR that forces the highlight.js auto highlighting mode on. That seems to be the stress test here.

Perfect! I was just about to ask if you had a good test case...

@phil-scott-78
Copy link
Contributor Author

Ok, updated #442 with a new test case with commit 4720180. It's a simple one, but definitely the stress test of a JS engine I think due to how highlight.js guesses the language for highlighting. Chances are with Jurasic or Jint you'll see exceptions like unterminated [] and other things due to them not being able to transform the regex into a .NET format.

@daveaglick
Copy link
Member

Can confirm your expected error with Jurassic:

JavaScriptException: SyntaxError: Invalid regular expression - parsing "[]{}%#'"" - Unterminated [] set.

The search continues...

@phil-scott-78
Copy link
Contributor Author

Yeah, I think JsEngineSwitcher is the only way to go here. I've got like 90% of an implementation going where I've abstracted it out so that it is only in the Core library. Need to regroup my thoughts on testing with it though.

@daveaglick
Copy link
Member

Might be, but I'm not ready to give up on a fully xplat answer quite yet 😉

I'm going to play around with Jint a little bit and see if I can't figure out how to fix/swap the regex handling. This looks like a pretty specific edge case with character class escapes, which is good because it hopefully means there's a consistent mechanical fix. It's not just .NET either - PCRE/PHP can't handle that expression either: https://regex101.com/r/FguKcY/1

In any case, I really like the idea of exposing a JS runner abstraction through Wyam.Common/Wyam.Core. That way it doesn't matter which engine we use and we can swap it out later if we need to. I'm interested to see what you came up with as far as the abstraction API (no rush though - ATM I'd just like to see if I can get one of the managed engines to work).

@daveaglick
Copy link
Member

AHA!

Got to the bottom of this. The problem is the [] part of the RegEx. That's actually not really valid. JavaScript does have support for "empty character classes" but it always matches nothing. In other words, even though it's valid syntax in JavaScript regexes, it's probably not intended. In fact, eslint actually has a rule to make sure you don't accidentally add an empty character class to your RegEx.

So since this looks like a bug inside highlight.js I was wondering where it was coming from. It's in the Ada language highlighter. Pretty sure that one's not too critical for us.

I also found one instance of another similar JavaScript-only character class [^] which matches any character (essentially it means "match the opposite of nothing"). That one is in the LISP highlighter. Those cases can be safely replaced with [\S\s] ("match either whitespace or not whitespace", in other words, everything).

By deleting the [] character class and replacing the [^] character class with [\S\s], highlight.js runs with Jint:

2017-02-14_23h13_54

So here's what I propose:

  • We build an abstraction around JS evaluation into Wyam.Common and use Jint to implement it in Wyam.Core
  • We use the patched version of highlight.js for now (attached - highlight-all.zip)
  • I'll submit a PR to Jint to make these two substitutions internally when processing regular expressions

@phil-scott-78
Copy link
Contributor Author

Well hot damn. That's fantastic.

@phil-scott-78
Copy link
Contributor Author

Had a chance to play with this some last night, but ran into some more roadblocks. Might be poor JS skills on my part, but I do fear this might fall into the not possible land without some serious hacks.

Problem 1 - sass.js uses ES2015. Jint only supports ES5 right now (although it looks like Jint 3.0 will support ES2015 which is nice). Messed around with a couple of polyfills like babel-es6-polyfill and es6-shim but both of them seem to make assumptions that I believe Jint just isn't having. es6-shim seems like a better option if one was going to go that route, but I think using something like babel to just transpile into es5 might be the more optimal solution. But before taking a C library that's been transpiled into ES2015 and transpiling that into es5 I wanted to make sure I could get the library working at all.

I added ChakraCore to the test projects which has ES2016 support and ran into the next hurdle. Looks like sass.js does some detection trying to figure out what environment it is in. It guesses we are a shell operation (good!) but it expects print to be a valid function. No problem, added that in manually and things moved forward and then it expected setTimeout to work. That...that is problematic. Best I can tell it's a combination of callbacks, setTimeout...basically a bunch of stuff node has some work arounds but I suspect might need to be implemented from scratch here.

And then the third problem - file system access. Need someway to plug into their callbacks so that we can hold it's hands when include calls are made and feed it the right file. Not sure there's a good way to do that if we can get past the previous two.

So long story short :-). I went hunting for other libraries but even in the node world the best solution is also a wrapper for libsass. It looks like libsass.net is tracking better support for Core support here. Might be wise just to rely on it rather than trying to force the issue with javascript IMO

This more or less copies the LESS module but adds Sass processing using
LibSass.Net. LibSass.Net is a wrapper around the C library LibSass which
worried me a bit, but everything seems to wire up just fine.

The test project is pretty simplistic. I would have liked to have a few
test cases for testing out that I got the include functionality right, but
I couldn't wrap my head around how to do that easily. But basic
verification of a successful call and a failing call are there for now.
@phil-scott-78
Copy link
Contributor Author

I never made much progress with getting sass.js to work. My javascript skills just aren't there. Figured I'd at least get this PR up to date though.

Original code was modifying the class level list on each run in parallel.
Not smart. This creates a new array if it is needed rather than adding to
the class level instance.
@daveaglick
Copy link
Member

Cool, thanks. I'm aiming for a release early next week with basically what's already committed. After that's out I'll take a look at this one and see if I can help at all - would love to get Sass support with the new Bootstrap on the horizon.

@phil-scott-78
Copy link
Contributor Author

Looks like an alternative library that is on the path to .NET core support is LibSassHost. Tricky part here is that it still requires the native libraries for Libsass and there are four separate packages for each platform. 😕

@phil-scott-78
Copy link
Contributor Author

I'm not letting this go easy into that good night 😄

spent way too much time doing server side sass processing for a side project. LibSassHost seems to be the better option here, and looks to be the best cross platform option. I think it might just be a matter of bringing in the root library and all four of the platform assemblies. It should just find the right one at runtime and be good to go.

The next issue would be that this would also require VC++ 2015 runtimes to be installed though. That's going to be an issue no matter what libsass wrapper is used. Not sure what could be done there other than fail gracefully and point them towards the installer (or choco package). I think the target audience for wyam more than likely does have it installed anyways, but it is definitely another stumbling block.

@daveaglick daveaglick force-pushed the develop branch 2 times, most recently from 439cbec to 340dd18 Compare April 20, 2017 20:30
@daveaglick daveaglick merged commit e67aeb5 into statiqdev:develop May 2, 2017
@daveaglick
Copy link
Member

Thanks again for the hard work on this one! I just merged it in and then did some refactoring, but the head start on the Sass module was much appreciated. Ended up being able to keep the entire public API you wrote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants