Skip to content

fix: resolve @import with absolute paths#201

Merged
joshwiens merged 2 commits intowebpack:masterfrom
n1ru4l:fix-import-absolute-path
May 30, 2017
Merged

fix: resolve @import with absolute paths#201
joshwiens merged 2 commits intowebpack:masterfrom
n1ru4l:fix-import-absolute-path

Conversation

@n1ru4l
Copy link
Copy Markdown
Contributor

@n1ru4l n1ru4l commented May 25, 2017

No description provided.

@jsf-clabot
Copy link
Copy Markdown

jsf-clabot commented May 25, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link
Copy Markdown

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
Copy Markdown
Contributor

michael-ciniawsky commented May 25, 2017

Fixes #93

@michael-ciniawsky
Copy link
Copy Markdown
Contributor

@n1ru4l Please add a test for this

Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor Author

n1ru4l commented May 25, 2017

@bebraw I will try that!

@n1ru4l
Copy link
Copy Markdown
Contributor Author

n1ru4l commented May 26, 2017

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

Copy link
Copy Markdown
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