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: Export a decoupled Sass importer #874

Merged
merged 8 commits into from Aug 24, 2020

Conversation

@vvanpo
Copy link
Contributor

@vvanpo vvanpo commented Aug 2, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

See #873

@jsf-clabot
Copy link

@jsf-clabot jsf-clabot commented Aug 2, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

@codecov codecov bot commented Aug 2, 2020

Codecov Report

Merging #874 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
src/utils.js 97.61% <100.00%> (+0.18%) ⬆️

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 d873b15...6a93f15. Read the comment docs.

@vvanpo vvanpo force-pushed the vvanpo:decoupled-importer branch from 7005704 to b7a455e Aug 2, 2020
src/importer.js Outdated Show resolved Hide resolved
@vvanpo vvanpo force-pushed the vvanpo:decoupled-importer branch from f0ae609 to 18bb883 Aug 5, 2020
vvanpo added 4 commits Aug 2, 2020
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 #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 vvanpo force-pushed the vvanpo:decoupled-importer branch from 18bb883 to c649db0 Aug 6, 2020
vvanpo added 2 commits Aug 15, 2020
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 vvanpo force-pushed the vvanpo:decoupled-importer branch from 93176d3 to 18106c3 Aug 16, 2020
@vvanpo
Copy link
Contributor Author

@vvanpo vvanpo commented Aug 16, 2020

@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 getWebpackResolver function separately, not only to fix the coverage regression but also to give us some protection against that function's API being accidentally changed in future refactors.

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]);

This comment has been minimized.

@alexander-akait

alexander-akait Aug 17, 2020
Member

No need await for return, just remove await

This comment has been minimized.

@vvanpo

vvanpo Aug 18, 2020
Author Contributor

If I don't await the promise, the try ... catch block will never catch any promise rejections, so a lot of tests fail.

This comment has been minimized.

@alexander-akait

alexander-akait Aug 24, 2020
Member

I will do refactor in future ()

Copy link
Member

@alexander-akait alexander-akait left a comment

Sorry for delay, hope we don't break something

@alexander-akait alexander-akait merged commit d2b532c into webpack-contrib:master Aug 24, 2020
22 checks passed
22 checks passed
@github-actions
Lint - ubuntu-latest - Node v12.x
Details
@github-actions
Test - ubuntu-latest - Node v10.x, Webpack latest
Details
@github-actions
Test - ubuntu-latest - Node v10.x, Webpack next
Details
@github-actions
Test - ubuntu-latest - Node v12.x, Webpack latest
Details
@github-actions
Test - ubuntu-latest - Node v12.x, Webpack next
Details
@github-actions
Test - ubuntu-latest - Node v14.x, Webpack latest
Details
@github-actions
Test - ubuntu-latest - Node v14.x, Webpack next
Details
@github-actions
Test - windows-latest - Node v10.x, Webpack latest Test - windows-latest - Node v10.x, Webpack latest
Details
@github-actions
Test - windows-latest - Node v10.x, Webpack next Test - windows-latest - Node v10.x, Webpack next
Details
@github-actions
Test - windows-latest - Node v12.x, Webpack latest
Details
@github-actions
Test - windows-latest - Node v12.x, Webpack next
Details
@github-actions
Test - windows-latest - Node v14.x, Webpack latest
Details
@github-actions
Test - windows-latest - Node v14.x, Webpack next
Details
@github-actions
Test - macos-latest - Node v10.x, Webpack latest
Details
@github-actions
Test - macos-latest - Node v10.x, Webpack next
Details
@github-actions
Test - macos-latest - Node v12.x, Webpack latest
Details
@github-actions
Test - macos-latest - Node v12.x, Webpack next
Details
@github-actions
Test - macos-latest - Node v14.x, Webpack latest
Details
@github-actions
Test - macos-latest - Node v14.x, Webpack next
Details
@codecov
codecov/patch 100.00% of diff hit (target 97.94%)
Details
@codecov
codecov/project 98.06% (+0.11%) compared to d873b15
Details
licence/cla Contributor License Agreement is signed.
Details
@alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Aug 24, 2020

I will not changelog it, because it is internal

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

Successfully merging this pull request may close these issues.

None yet

3 participants