Switch to dart-sass.

[XhmikosR]

Need to double check the generated CSS, but this has a lot less dependencies.

[XhmikosR]

This seems to be fine.

1. it's a little slower but that's a known thing. The sync method is supposed to be faster
2. this is the standard sass implementation
3. precision is hardcoded to 10 in dart-sass

The final CSS diff https://gist.github.com/XhmikosR/dffbd893f193cb21be9933459d3bd06b/revisions

---

1109 vs 1014 packages and

https://packagephobia.now.sh/result?p=node-sass
https://packagephobia.now.sh/result?p=sass

[XhmikosR]

Can I get some reviews here please?

@nschonni@Trott: this seems like a huge win to me, unless I'm missing a dart-sass issue :)

[Trott]

LGTM but @nschonni has lots of contributions into node-sass (and one into dart-sass) so I defer to them as the expert here.

[nschonni]

I'm mostly -0 on this because it's probably more important for the redesigned website setup than here where it's working. node-sass is faster, but since it's a native add-on, it has the install quirks that go with that. Unless you're looking at leveraging feature like the module system in here which is only available in dart-sass, I don't see the point of swapping

[XhmikosR]

Well, its install size and the fact that we compile it on each build is enough reasons for me. Assuming dart-sass does not have the same problem with binaries when deploying, it's definitely an easy win. I see no reason to keep using a 8x bigger package for a few milliseconds that takes to build css,when the whole build takes hundreds of seconds.

…

On Wed, Oct 30, 2019, 22:17 Nick Schonning ***@***.***> wrote: I'm mostly -0 on this because it's probably more important for the redesigned website setup than here where it's working. node-sass is faster, but since it's a native add-on, it has the install quirks that go with that. Unless you're looking at leveraging feature like the module system in here which is only available in dart-sass, I don't see the point of swapping — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2724?email_source=notifications&email_token=AACVLNIJQPZH5VFV4FR7EBDQRHTWTA5CNFSM4JFAFXXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECVT7LQ#issuecomment-548093870>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNMMETH2EDVGA5FGOYLQRHTWTANCNFSM4JFAFXXA> .

[nschonni]

the fact that we compile it on each build

It should be unless it's running on an unsupported platform (ARM, s390, etc...), and the downloads get cached after the first install

[XhmikosR]

I don't think it is. It's built on each deployment AFAICT.

…

On Wed, Oct 30, 2019, 22:32 Nick Schonning ***@***.***> wrote: the fact that we compile it on each build It should be unless it's running on an unsupported platform (ARM, s390, etc...), and the downloads get cached after the first install — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2724?email_source=notifications&email_token=AACVLNOOOWCIMID3NDFNQPDQRHVM3A5CNFSM4JFAFXXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECVVNIA#issuecomment-548099744>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNJAKWNJQFO7ITZYRQDQRHVM3ANCNFSM4JFAFXXA> .

[nschonni]

I think that was just in that webhook log between the Node 13 release and the node-sass 4.13 one.
This is from the latest install

stdout: Adding user `nodejs' ...
stdout: Adding new user `nodejs' (1001) with group `nodejs' ...
stdout: Creating home directory `/home/nodejs' ...
stdout: Copying files from `/etc/skel' ...
stderr: npm http fetch POST 200 https://registry.npmjs.org/-/npm/v1/security/audits/quick 1524ms
stdout: stdout: > node-sass@4.13.0 install /website/node_modules/node-sass
stdout: > node scripts/install.js
stdout: stdout: Cached binary found at /npm/node-sass/4.13.0/linux-x64-79_binding.node
stdout: stdout: > node-sass@4.13.0 postinstall /website/node_modules/node-sass
stdout: > node scripts/build.js
stdout: stdout: Binary found at /website/node_modules/node-sass/vendor/linux-x64-79/binding.node
stdout: Testing binary
stdout: Binary is fine
stdout: added 392 packages from 292 contributors and audited 4501 packages in 8.609s
stdout: found 1 low severity vulnerability
stdout: run `npm audit fix` to fix them, or `npm audit` for details
stdout: stdout: > nodejs.org@ deploy /website
stdout: > npm-run-all load-schedule build:deploy external:survey gzip

[XhmikosR]

Perfect, then it's one thing less to worry about.

That being said, still a decrease of 8x in size, and -100 deps is still a win.

Note that I the Sass switch was my patch, and the reason I didn't went with dart-sass back then is because I hadn't played with it yet. If I were to make the switch now, I'd make the patch with dart-sass.

[XhmikosR]

BTW I still see https://nodejs.org/github-webhook.log Not sure why it says

stderr: Previous HEAD position was 34053e0... Blog: v13.0.1 release post (#2718)
stderr: HEAD is now at cdcac6a... chore: Remove pre-commit (#2711)

when we have pushed more in master.

[XhmikosR]

@rvagg ^^

[rvagg]

the log keeps getting appended to until it's rotated, scroll right down, I see stdout: HEAD is now at d1da578 Add .netlify in .gitignore (#2728) in there, that's the latest HEAD commit here.

[XhmikosR]

Oops, sorry, I didn't see that :)

Ok, so all good then.

[Trott]

@nodejs/website Anyone have informed opinions on this?

[XhmikosR]

I guess you mean apart from me :) #2724 (comment)

[Trott]

I guess you mean apart from me :) #2724 (comment)

Yes, "someone who is not the change author". 😀

[silverwind]

This is the first time I hear about dart-sass and I think I'm +0 here. I prefer to avoid native modules so the fact that it's compiled to pure JS is nice. I could forsee many projects switchting to it if it's the reference implementation. On the other hand, I'm not sure how to feel about Dart itself.

Probably not a good indicator, but it currently has 1/4 the downloads of node-sass but is gaining traction:

https://www.npmjs.com/package/sass
https://www.npmjs.com/package/node-sass

[XhmikosR]

The only things I've seen so far is:

1. the async implementation is slower that node-sass async and we need to use the fibers package; not really important in our case here since the build process is slow due to the amount of files we build, not sass
2. sass has a hardcoded precision of 10; this doesn't affect us here, though
3. the ~-100 packages is a huge win for me, especially since the minified output is pretty close to what we had before here (see the beautified diff https://gist.github.com/XhmikosR/dffbd893f193cb21be9933459d3bd06b/revisions)

So, AFAICT this is a win-win situation.

[XhmikosR]

Ping @nodejs/website. See my comments above. I really want to move with this, since the less deps, the better.

[XhmikosR]

@rvagg would it be possible to rotate https://nodejs.org/github-webhook.log more frequently? It becomes way too big.