fix(index): don't prepend ./ to the URL on interpolate=require (options.interpolate)
#165
Conversation
This fixes bug #150 where the URL passed to `require()` would have `./` prepended when it shouldn't.
|
|
|
LGTM |
index.js
Outdated
| } else { | ||
| urlToRequest = loaderUtils.urlToRequest(data[match], root); | ||
| } | ||
|
|
||
| return '" + require(' + JSON.stringify(loaderUtils.urlToRequest(data[match], root)) + ') + "'; |
There was a problem hiding this comment.
- return '" + require(' + JSON.stringify(loaderUtils.urlToRequest(data[match], root)) + ') + "';
+ return '" + require(' + JSON.stringify(urlToRequest) + "';? Or I'm missing something, but it looks like urlToRequest isn't used atm :)
There was a problem hiding this comment.
I was trying to keep existing functionality as sometimes config.interpolate could be true instead of require. I only wanted to change this for the instance I know it's broken.
There was a problem hiding this comment.
How is the variable urlToRequest applied ? From looking at the code it seems to always return
'" + require(' + JSON.stringify(loaderUtils.urlToRequest(data[match], root)) + ') + "';atm ? So your changes don't apply ?
There was a problem hiding this comment.
You're correct; my mistake! I made these changes in my local node_modules directory and didn't copy them over correctly when creating the pull request.
michael-ciniawsky
changed the title
./ to url./ to the URL on interpolate=require (options.interpolate)
michael-ciniawsky
changed the title
./ to the URL on interpolate=require (options.interpolate)./ to the URL on interpolate=require (options.interpolate)
index.js
Outdated
| } else { | ||
| urlToRequest = loaderUtils.urlToRequest(data[match], root); | ||
| } | ||
|
|
||
| return '" + require(' + JSON.stringify(loaderUtils.urlToRequest(data[match], root)) + ') + "'; |
There was a problem hiding this comment.
You're correct; my mistake! I made these changes in my local node_modules directory and didn't copy them over correctly when creating the pull request.
|
I put in the required change to make this function properly. Do you know the ETA on when this would work its way into the codebase? |
michael-ciniawsky
added
status: Approved
and removed
pr_action: Rework
priority: 4 (important)
severity: 3 (broken)
status: Pending
labels
|
Released in v0.5.5 🎉 |
This fixes bug #150 where the URL passed to
require()would have./prepended when it shouldn't.Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
#150
What is the new behavior?
require, when interpolated, will no longer prepend./to the request url.Does this PR introduce a breaking change?
If this PR contains a breaking change, please describe the following...
Other information: