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

fix: resolve @import with absolute paths #201

Merged
merged 2 commits into from
May 30, 2017

Conversation

n1ru4l
Copy link
Contributor

@n1ru4l n1ru4l commented May 25, 2017

No description provided.

@jsf-clabot
Copy link

jsf-clabot commented May 25, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented May 25, 2017

Codecov Report

Merging #201 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #201   +/-   ##
=======================================
  Coverage   97.93%   97.93%           
=======================================
  Files           7        7           
  Lines          97       97           
  Branches        8        9    +1     
=======================================
  Hits           95       95           
  Misses          2        2
Impacted Files Coverage Δ
src/createWebpackLessPlugin.js 100% <100%> (ø) ⬆️

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 9ed895f...c1d0980. Read the comment docs.

@michael-ciniawsky michael-ciniawsky changed the title - Fixes webpack-contrib/less-loader#93 fix: resolve @import with absolute paths May 25, 2017
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented May 25, 2017

Fixes #93

@michael-ciniawsky
Copy link
Member

@n1ru4l Please add a test for this

Copy link

@bebraw bebraw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a test.

I wonder if it would be better to fix loaderUtils.urlToRequest call (possible?). That would push complexity elsewhere, though. See https://github.com/webpack/loader-utils/blob/master/lib/urlToRequest.js .

@n1ru4l
Copy link
Contributor Author

n1ru4l commented May 25, 2017

@bebraw Since the class is encapsulated inside a function this seems pretty hard to test for me :/. Any suggestions how I should proceed?
@bebraw I think this can be achieved by loaderUtils.urlToRequest(url, '')

@bebraw
Copy link

bebraw commented May 25, 2017

@n1ru4l Yeah, I think it would take some refactoring (extracting class?) to test this. But it's worth doing to avoid breaking the feature in the future.

@n1ru4l
Copy link
Contributor Author

n1ru4l commented May 25, 2017

@bebraw I will try that!

@n1ru4l
Copy link
Contributor Author

n1ru4l commented May 26, 2017

@bebraw I added a test that does not require any refactoring

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good!

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

Successfully merging this pull request may close these issues.

6 participants