search

bholloway / resolve-url-loader

Webpack loader that resolves relative paths in url() statements based on the original source file
563 stars 71 forks source link

Resolved paths no longer contain backslash path separators on windows #6

Closed mrsharpoblunto closed 8 years ago

mrsharpoblunto commented 8 years ago

Having backslashes in url paths is invalid, and causes problems when used in combination with other webpack loaders such as css-loader as css-loaders post-css transform seems to remove backslashes from paths causing resolved paths to change from .....\myfile to .....myfile.

This patch fixes this problem by ensuring that all backslashes in resolved urls are converted into forward slashes.

bholloway commented 8 years ago

Awesome @mrsharpoblunto, thanks for the PR.

Previously I have just used slash but happy to go with the RegEx, whichever you prefer.

But there are some aspects we may need to include.

Let me know what you think.

mrsharpoblunto commented 8 years ago

Regarding the points you brought up

Extended length paths/non ascii - Slash excludes those two cases, because forward slashes are not valid in windows paths in those two cases, however because we're not outputting windows filesystem paths (we're outputting uri paths) I don't think this restriction applies in resolve-url-loaders case. In fact - any time we don't convert slashes is going to cause problems with things like css-loader.

Global regex flag - The regex in my patch does include the global flag, or were you suggesting that it shouldn't have the flag set? The regex definitely should have the global flag set so that we replace all path separators rather than just the first one.

bholloway commented 8 years ago

Hey yeah, completely misread the global flag. Oops.

Regarding the extended paths I don't think we can be really sure anyway until these corner cases come up in practice. So lets go with what you've got.

bholloway commented 8 years ago

Regarding release. Do you feel this is patch or minor release?

i.e. is there possibility this could change functionality for existing users. I'm thinking not, implying just a patch release.

mrsharpoblunto commented 8 years ago

I think you're right, its probably ok as a patch release. Technically, it changes functionality for windows users, but considering it didn't seem to work at all with backslash paths I think its probably ok.

bholloway commented 8 years ago

Published as resolve-url-loader@1.3.1

Thanks @mrsharpoblunto, pleasure working with you.

mrsharpoblunto commented 8 years ago

Cool, thanks @bholloway for the super fast response and update :)