feat: Export a decoupled Sass importer #874
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #874 +/- ##
==========================================
+ Coverage 97.94% 98.06% +0.11%
==========================================
Files 4 4
Lines 195 207 +12
Branches 65 67 +2
==========================================
+ Hits 191 203 +12
Misses 4 4
Continue to review full report at Codecov.
|
vvanpo
force-pushed
the
decoupled-importer
branch
from
7005704
to
b7a455e
Compare
vvanpo
force-pushed
the
decoupled-importer
branch
from
f0ae609
to
18bb883
Compare
This will be used by `vue-jest` and potentially other projects, to write a Jest transform that can adequately mimic `sass-loader`'s behaviour. Closes webpack-contrib#873
As discussed in code review, this is more the responsibility of the consumer, as `sass-loader/dist/utils.js` already exports all the necessary functionality.
vvanpo
force-pushed
the
decoupled-importer
branch
from
18bb883
to
c649db0
Compare
We can reduce our API surface slightly by not considering `getSassImplementation` as a public function, and instead using to determine defaults from within the only public function in `utils.js`: `getWebpackResolver()`.
Because `getWebpackResolver` is now part of `utils.js`' public API, we should test it separately so that we have some protection against its API changing drastically from potential future refactors.
vvanpo
force-pushed
the
decoupled-importer
branch
from
93176d3
to
18106c3
Compare
|
@evilebottnawi I had been ignoring the broken build because the only regression in code coverage was due to two parameters being made optional, and not calling them specifically without arguments gets reported as missing branch coverage. But I realize now that you were probably waiting on me to fix the build. So I've added some tests to test the |
Using `getSassImplementation()` inside the resolver factory is not a good idea not only because it means it will be called twice for normal usage of the loader, but also because consumers of `getWebpackResolver` are almost certainly also calling Sass itself, meaning they'll need to use `getSassImplementation()` anyway to make sure they're using the same implementation as the resolver thinks is being used.
| try { | ||
| result = await resolve(context, possibleRequests[0]); | ||
| return await resolve(context, possibleRequests[0]); |
There was a problem hiding this comment.
No need await for return, just remove await
There was a problem hiding this comment.
If I don't await the promise, the try ... catch block will never catch any promise rejections, so a lot of tests fail.
There was a problem hiding this comment.
I will do refactor in future ()
|
I will not changelog it, because it is internal |
This PR contains a:
Motivation / Use-Case
See #873