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

feat: Use sass rather than node-sass #16

Merged
merged 4 commits into from
Apr 7, 2020
Merged

feat: Use sass rather than node-sass #16

merged 4 commits into from
Apr 7, 2020

Conversation

timfish
Copy link
Contributor

@timfish timfish commented Apr 1, 2020

node-sass will fail to install when:

  • Platforms where pre-built binaries are not supplied
  • Build from source fails due to missing native build tools or no Python 2

Even with Python 2 and build tools installed, building node-sass from source takes minutes on a Raspberry Pi 4.

For our large project there is no noticeable speed difference when using sass instead of node-sass and sass-loader picks it up automatically.

@timfish timfish changed the title feat: Use sass rather than node-sass for webpack feat: Use sass rather than node-sass Apr 1, 2020
@zewa666
Copy link
Member

zewa666 commented Apr 1, 2020

Doesnt that require ruby? I've used https://www.npmjs.com/package/gulp-dart-sass which makes use of dart-sass which works flawlessy.

@timfish
Copy link
Contributor Author

timfish commented Apr 1, 2020

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

This package is a distribution of Dart Sass, compiled to pure JavaScript with no native code or external dependencies. It provides a command-line sass executable and a Node.js API.

@3cp
Copy link
Member

3cp commented Apr 1, 2020

Most users are not on an ARM Linux. I am reluctant to fix this issue because it's irrelevant to most users, and it did make the setup look strange.

On the other side, webpack's sass-loader does auto-detection of node-sass and sass. It could be better if gulp-sass does the same. dlmanning/gulp-sass#748

If gulp-sass ships that breaking change, we will definitely convert both webpack and dumber setup to use dart sass since it provides maximum compatibility.

@3cp
Copy link
Member

3cp commented Apr 1, 2020

From sass official site, I am bit surprised to know that the original Ruby sass is deprecated since March 2019. Dart sass is now the reference implementation, Libsass (node-sass uses) is the secondary implementation.

That means we need to move to dart sass.

@timfish
Copy link
Contributor Author

timfish commented Apr 1, 2020

and it did make the setup look strange

I thought that until I read the gulp-sass docs which say that it's "strongly recommended" that you set the sass.compiler property "for forwards-compatibility in case the default ever changes". So even if you stick with node-sass you should probably be setting sass.compiler = require("node-sass").

@3cp
Copy link
Member

3cp commented Apr 1, 2020

My point is not to stick with anything. My point is gulp-sass should be smarter, and it can be, as my comment in that gulp-sass PR.

I agree we should move to dark-sass, just want to hold this PR for few days for some opinions. I don't expect that gulp-sass PR to be merged, as the author never responded on any PRs.

@timfish
Copy link
Contributor Author

timfish commented Apr 1, 2020

Ah yes, I failed to notice that node-sass is actually a dependency of gulp-sass so npm install can still fail on unsupported platforms even if this PR is merged. 🤷‍♂️

@3cp
Copy link
Member

3cp commented Apr 1, 2020

That's too bad. If gulp-sass didn't merge the PR, we will use gulp-dart-sass (but it's not very active).

Or write a new gulp plugin for sass. It's trivial, and I can cut off lots of unneeded deps (chalk, lodash, node-sass, replace-ext, strip-ansi, through2), remove async mode because sync mode is twice faster. The downside is I need to maintain it.

@3cp
Copy link
Member

3cp commented Apr 2, 2020

gulp-dart-sass should work for you. What about update this PR to use it instead?

@timfish
Copy link
Contributor Author

timfish commented Apr 2, 2020

I checked gulp-dart-sass and it's dependencies were out of date and the tests wouldn't run. I've submitted a PR and once that's merged I'll update this one.

@3cp
Copy link
Member

3cp commented Apr 2, 2020

All you gulp-dart-sass pr updated are devDeps, the one (sass) in deps doesn't count because it's not major version update.

That pr would not affect anything here, because devDeps is irrelevant when using gulp-dart-sass.

All the CI tests in this pr passed, including few e2e tests on real apps. (I mistook this repo with aurelia-cli repo, btw we will update aurelia-cli to dart sass too) What is your error in your app using gulp-dart-sass?

If you were talking about the test in gulp-dart-sass project, I think that's not a broker for us.

@3cp
Copy link
Member

3cp commented Apr 3, 2020

Great, update to gulp-dart-sass v1 pls.

@timfish
Copy link
Contributor Author

timfish commented Apr 7, 2020

@3cp I've now updated gulp-dart-sass to ^1.0.0.

The test failures appear to be related to being unable to install node:
image

@3cp
Copy link
Member

3cp commented Apr 7, 2020

Thx. I will run some selected e2e tests.

@3cp 3cp merged commit 51ffd74 into aurelia:master Apr 7, 2020
@timfish timfish deleted the sass branch April 8, 2020 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants