fix #5826; swap node-sass for dart-sass

[tomByrer]

Description

Per #5826, node-sass is difficult to install sometimes. Dart-sass is the new 'standard'.

Specific Changes proposed

To get dart-sass to work, I had to fix the path to node_modules for @import (which is backward compatible with node-sass) & remove the --watch folder.

Testing

Works in Win10 & OSX Sierra, both using VSCode-git-bash & npm. Direct npm run build:css & npm start works, as does tests in Firefox & Safari. Compiling seems to be ~5-10% slower than node-sass, but we're only talking milliseconds.

I haven't tested editing an scss file & see if the --watch works (I assume runs only for the main entry & @imported files).

Thoughts

I have some reservations. Dart-sass is clearly a 1-woman show. Perhaps the spec changes (as few a there are now) is handled by a team, she writes 99% of the code. Amazing work, but I question long-term maintenance. Documentation has not been updated, with references to ruby-sass which is about to mothballed in weeks.

While LibSass may never be 100% reference, it became the de-facto standard. Node-sass has some minor behavior differences.

So, up to you guys if you want to make the switch; there are pros & cons.

Requirements Checklist

• Feature implemented / Bug fixed
• If necessary, more likely in a feature request than a bug fix
  • Change has been verified in an actual browser (Chome, Firefox, IE)
  • Unit Tests updated or fixed
  • Docs/guides updated
  • Example created (starter template on JSBin)
• Reviewed by Two Core Contributors

[gkatsev]

Thanks for the work. 5-10% slowness is probably fine to mean that we aren't depending on a native module anymore.
I'll take some time next week to take this for a spin.

[gkatsev]

what's the difference between dart-sass and sass? Seems like documentation mostly talks about sass.

[tomByrer]

Seems to be the exact same code, but double-listed in npm under different project names. Note the version, 'last publish', GitHub stats, etc:
https://www.npmjs.com/package/sass
https://www.npmjs.com/package/dart-sass

I chose dart-sass to be more clear about the origin. Both seemed to work the same when I was doing some wiring.

[brandonocasey]

Since the docs for dart-sass specify the sass package, I think we should go with that.

[tomByrer]

Any luck in your findings @gkatsev ?

[gkatsev]

I haven't had much time to look at it further.

Also, given that all the documentation says to use the sass module, I'd probably prefer that, even if they are otherwise equivalent.

[brandonocasey]

I did some testing with this today:

Over 20 runs of npm run build:css with dart-sass it took: 3.44865s
Over 20 runs of npm run build:css with node-sass it took: 2.8767s
So a difference of ~16.5% or 0.57195s between the two

From a quick visual test on safari/chrome/firefox everything looks the same, but we probably want to test a lot of specifics since the diff has a lot of non-whitespace changes.

Here is a diff between the two that I did (where node-sass generated css, is being changed to dart-sass css)
eb852c4?w=1

[gkatsev]

Thanks @brandonocasey! Looking a bit more carefully, some of the non-whitespace changes are just a change in the ordering of the selectors, for example:

.vjs-icon-play, .video-js .vjs-big-play-button .vjs-icon-placeholder:before, .video-js .vjs-play-control .vjs-icon-placeholder {}

was changed to:

.vjs-icon-play, .video-js .vjs-play-control .vjs-icon-placeholder, .video-js .vjs-big-play-button .vjs-icon-placeholder:before {}

The difference is that before, vjs-big-play-button was first with vjs-play-control second but then they swapped.

@brandonocasey would you be able to run something like this replace on the output: s/\n}/}/? Basically, delete the new line before closing braces, that should minimize the changeset.

[brandonocasey]

yeah let me see what I can do, I will re-post the diff after fixing some whitespace issues.

[brandonocasey]

A bit better: #5903

[gkatsev]

Yeah, looks like it's mostly switching around the selectors or putting them on one line. We'll want to double check that it's true for all the changes, though. But seems like this can be made in a minor update for sure.

[brandonocasey]

OK, I checked all the changes, and found three differences that I made comments on in that pull request. Seems like rounding is a bit different.

[gkatsev]

Hiding whitespace changes makes that other PR a lot more manageable.
I don't think that the precision change matters enough to warrant further investigation. Going from 15 3's to 10 3's is pretty much still just a bunch of 3s.

[gkatsev]

Also, we probably would want to turn off sourcemaps.

[gkatsev]

So, what's I think there are a couple of things left here:

• switch to the sass package
• disable sourcemaps
• rebase against master and fix package-lock conflicts

[gkatsev]

@tomByrer would you be able to finish the tasks outlines in my last comment?

[gkatsev]

I've created a new PR ( #6055 ) which dones the above tasks.