feat: add dart-sass support default

[xiaoxiangmoe]

No description provided.

[jsf-clabot]

All committers have signed the CLA.

[alexander-akait]

Why? You need accept CLA and fix tests

[xiaoxiangmoe]

@evilebottnawi fixed.

[alexander-akait]

@xiaoxiangmoe Why we should switch on dart-sass right now?

[xiaoxiangmoe]

@evilebottnawi

This code will use node-sass as the default implementation.

Dart-sass is used when node-sass is missing and dart sass is installed. So there are no destructive updates.

https://sass-lang.com/dart-sass

Dart Sass is the primary implementation of Sass, which means it gets new features before any other implementation.

[codecov]

Codecov Report

Merging #626 into master will decrease coverage by 0.55%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
- Coverage   97.79%   97.24%   -0.56%     
==========================================
  Files           6        6              
  Lines         136      145       +9     
==========================================
+ Hits          133      141       +8     
- Misses          3        4       +1

Impacted Files	Coverage Δ
lib/loader.js	98.43% <88.88%> (-1.57%)	⬇️
lib/proxyCustomImporters.js	100% <0%> (ø)	⬆️
lib/webpackImporter.js	100% <0%> (ø)	⬆️
lib/normalizeOptions.js	96.42% <0%> (ø)	⬆️
lib/importsToResolve.js	100% <0%> (ø)	⬆️
lib/formatSassError.js	88.88% <0%> (+0.65%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2adcca3...ef7ecc2. Read the comment docs.

[alexander-akait]

Looks good, but it is breaking change, wen can merge this only for major release

[xiaoxiangmoe]

It isn't breaking change. It will still use node-sass if there exists node-sass without configuring the implementation.

This only affects users who have installed dart-sass without installing node-sass and without configuring the implementation.

If we want to switch to use dart-sass default. It is break change. Maybe we can checkout to it in next major release.

[alexander-akait]

@xiaoxiangmoe it is breaking change for people who don't install node-sass but have installed sass, previous it is output error, now it is loading sass, also need test for this

[xiaoxiangmoe]

Okay. @evilebottnawi Could you please tell me the plan for the release of the next major version? Thanks.

[alexander-akait]

fir problem #556 (comment) and mainField for current release and we can start new major release

[sibiraj-s]

If that is going in a next major version then there is no need for fallback. We can just reverse whatever we do now..

from

const render = getRenderFuncFromSassImpl(options.implementation || require("node-sass"));

to

const render = getRenderFuncFromSassImpl(options.implementation || require("sass"));

rite? It will be breaking however @xiaoxiangmoe @evilebottnawi

[alexander-akait]

@sibiraj-s what do you mean?

[sibiraj-s]

Hi @evilebottnawi .

Here for new installations dart-sass is considered/recommended as default instead of node-sass. A check is added to avoid breaking change for those who upgrade with node-sass installed.

You said that this PR will go in the next major release(considering 8.0.0). In that case, these fallback strategies implemented in this PR is not needed, since major release are expected to have breaking change. User can uninstall node-sass and install sass and start using it or add the implementation method to retain the functionality.

We can just reverse the existing method. Instead of looking for node-sass, now we require sass. No need for try/catch or anything.

[alexander-akait]

/cc @jhnns

[alexander-akait]

/cc @xiaoxiangmoe need investigate why coverage decrease

[alexander-akait]

WIP on this, will be released in sass-loader@7.2.0

[alexander-akait]

Close in favor #646, will be released in 7.2.0, anyway thanks for work and idea 👍

[xiaoxiangmoe]

@evilebottnawi When will 7.2.0 be released?

[alexander-akait]

@xiaoxiangmoe when it is ready for release, already in my todo list, ETA is around 1-2 weeks

[alexander-akait]

@xiaoxiangmoe many work, this month, sorry

[xiaoxiangmoe]

@evilebottnawi Excuse me. Do you need any help?

[alexander-akait]

@xiaoxiangmoe migrate test on jest and snapshoting