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

Handle periods in directory names when interpolating paths #71

Merged
merged 1 commit into from
Mar 6, 2017

Conversation

pugnascotia
Copy link
Contributor

Given:

  • A filename without an extension, and
  • A directory path containing a period

...the result from interpolateName is wrong. I've reworked the code and added some tests to cover this.

@jsf-clabot
Copy link

jsf-clabot commented Feb 24, 2017

CLA assistant check
All committers have signed the CLA.

@pugnascotia
Copy link
Contributor Author

Aw, no fair on the decreased coverage - it's because there's fewer lines of code :-(

@pugnascotia
Copy link
Contributor Author

Maintainers - I can add tests to increase the coverage score, but that will increase the scope of these changes. What would you like me to do?

@Timer
Copy link

Timer commented Mar 1, 2017

Hi!

We would love to see some feedback given on this issue so that it may be merged and released.
We're depending on it as part of our CRA react-scripts@0.10.0 release, which switches to webpack@2!
@pugnascotia has been working hard to give us a whitelisted url-loader and our comprehensive e2e uncovered this bug. 🔥

Let me know if there's anything I can do to help facilitate this process, and thanks to all parties!

/cc @jhnns

@jhnns jhnns merged commit aaff808 into webpack:master Mar 6, 2017
@jhnns
Copy link
Member

jhnns commented Mar 6, 2017

Thanks! That looks good. It makes sense to use path.parse() here since loaderContext.resourcePath is a native, absolute path, so node's parsing algorithm should work as expected. I don't know why @sokra preferred to parse it manually here 😬

Published as 1.0.3

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

Successfully merging this pull request may close these issues.

4 participants