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

Migrate from node-sass to dart-sass #366

Closed
jbalsas opened this issue Dec 23, 2020 · 4 comments · Fixed by #368
Closed

Migrate from node-sass to dart-sass #366

jbalsas opened this issue Dec 23, 2020 · 4 comments · Fixed by #368

Comments

@jbalsas
Copy link
Contributor

jbalsas commented Dec 23, 2020

Analog to Integrate dart-sass in liferay-npm-scripts build process.

LibSass was deprecated on October 2020. Migration steps point to:

If you’re a user of Node Sass, migrating to Dart Sass is straightforward: just replace node-sass in your package.json file with sass. Both packages expose the same JavaScript API.

So, hopefully this is an easy one! ;)

@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 23, 2020

So, hopefully this is an easy one! ;)

[narrator]It wasn't[/narrator]

Update gulp-sass

This should be the easiest path. In the 4.0.2 release they added the option to set your own sass compiler which is now documented in the Basic Usage section.

var gulpSass = require('gulp-sass');
var sass = require('sass')

gulpSass.compiler = sass;

You can choose whether to use Dart Sass or Node Sass by setting the sass.compiler property. Node Sass will be used by default, but it's strongly recommended that you set it explicitly for forwards-compatibility in case the default ever changes.

So, I did:

  • Upgrade gulp-sass to 4.1.0
  • Add sass as a dependency
  • Set sass as gulpSass.compiler prior to our invocation in build_css

Unfortunately, this yields the following compilation error:

Error: Can't find stylesheet to import.
@import 'clay/base';

This is weird because gulp-sass makes sure to add the parent folder to the paths the compiler sees so I'm not sure why sass can't resolve it now 🤔

Even if it worked, this is a subpar approach because node-sass remains a dependency of gulp-sass which means we are not removing it from our dependency list and we will still download it even after the switch. This takes us to the next approach I tried.

Use sass directly

This should be pretty straightforward as well. The gulp-sass project is basically a thin wrapper using through2 to pipe the sass processing which is almost trivial to inline in our own code getting rid of the unwanted dependencies. So I did just that :D

On top of it, I moved compilation to always be sync based on the following disclaimer in the gulp-sass project:

Note that when using Dart Sass, synchronous compilation is twice as fast as asynchronous compilation by default, due to the overhead of asynchronous callbacks. To avoid this overhead, you can use the fibers package to call asynchronous importers from the synchronous code path. To enable this, pass the Fiber class to the fiber option:

Unfortunately (and fortunately at the same time), the same compilation error emerges. That means that there's likely an issue with how we're configuring includePaths or something else in both cases and means that this second approach could work just fine with the added benefit of one less dependency.

/cc @bryceosterhaus, just in case you want to get away from the other sass thing for a little bit. I can push a PoC with what I have, but it's pretty much trivial at this point, so not sure if it's worth it.

@jbalsas
Copy link
Contributor Author

jbalsas commented Jan 18, 2021

Hey @bryceosterhaus, could you take this one over now that we have the other one about sass merged into master?

I'd say #368 is good, just need to rethink that one nasty test... maybe rethink it? 🤷

@bryceosterhaus
Copy link
Member

Yep! Was already looking into it, hopefully should have something concrete here in the next day or two

@jbalsas
Copy link
Contributor Author

jbalsas commented Jan 18, 2021

Yep! Was already looking into it, hopefully should have something concrete here in the next day or two

loveyou

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

Successfully merging a pull request may close this issue.

3 participants