Plugin with `include` or `exclude` options fails on Windows

[DHedgecock]

Using either the include or exclude options causes the plugin to fail on Windows. The plugin uses a string.search call to check if the requested module path matches either of the include/exclude sets, but using string.search implicitly creates an invalid regex on Windows machines due to the module path forward slashes.

[DHedgecock]

Here is an example repo with the issue: https://github.com/DHedgecock/dnwp-example-failure Repo builds on Mac, but fails on Windows.

Running npx webpack will result in:

ERROR in ./src/index.js
Module not found: Error: Can't resolve './Test' in 'C:\Users\username\Documents\GitHub\dnwp-example-failure\src'
 @ ./src/index.js 1:13-30

I took a quick run at making updates to the .search, including keeping it, but making the string regex safe, but the tests were failing. I'll take a quick look after work later (I don't know if the test setup where escape characters are included is a realistic example?).

[shaketbaby]

Thanks for reporting, I'll see how this can be fixed. In the meantime, I think you can provide a Regex to include/exclude that meets your requirement instead of a string. I'll try to fix this ASAP.

[asbjornh]

Any progress on this? :) If you don't have the time, are you accepting PRs? I may have time. I'm guessing that the problem here is that webpack uses unix-like paths internally regardless of OS or something like that?

In the meantime, I used regex like you suggested which works okay.

[shaketbaby]

Sorry, I don't really have time at the moment. PR is always very welcome.

[asbjornh]

@shaketbaby So I've poked around a bit in the source, and it seems like right now the plugin is built on the principle that even when using strings they should be treated as regular expression since String.search converts strings to regexp. The tests also seem to reflect this.

I was able to fix the above issue on windows by using String.includes for string patterns, but this breaks several tests.

I also tried comparing the path strings from webpack with exclude/include patterns built with path.resolve and they were an exact match even on Windows (character by character) even though the comparison failed. I also tried comparing the same strings (manually typed) in the node REPL on Windows, and the searches worked as expected.

I haven't been able to figure out why the search returns -1 with paths from webpack on Windows, which leads me to think that using String.includes is the simplest solution. This would be a breaking change though. What do you think?

[shaketbaby]

@asbjornh I would prefer not to introduce breaking changes if possible. String.includes treat the argument as plain string instead of a Regex. Maybe we could use both String.includes and String.search, if either of them returns true then consider it matches. Also an idea on why String.search return -1. Since path separator on Windows is \ which has special meaning in Regex, maybe it needs to be escaped to \\ before passing to String.search?

[asbjornh]

I guess both could work, yeah :) Would be a little more code but I think that should also solve the problem. I don't want to go down the rabbit hole of manipulating file paths :P

The weird part is that in the Node REPL on Windows the search works fine:

> "C:\A\B".search("C:\A");
0

[asbjornh]

Yep! Doing includes after search seems to solve the Windows-issue without breaking tests :)

[asbjornh]

I've added a fix here: https://github.com/asbjornh/directory-named-webpack-plugin

@DHedgecock would you mind testing this? You can do npm install asbjornh/directory-named-webpack-plugin

If you confirm that it's working I'll create a PR :)

[shaketbaby]

The weird part is that in the Node REPL on Windows the search works fine:

> "C:\A\B".search("C:\A");
0

In this example, since \ is also used as escaping character in String; after escaping, "C:\A\B" becomes "C:AB" and "C:\A" becomes "C:A". So it works. While the path from Webpack I think would be "C:\\A\\B" and the string passed to search would need to be "C:\\\\A" to match. I don't have a Windows around so can't verify that easily :-)

If it works on your Windows machine and you are confident with the change, feel free to create PR. And thanks for your effort, really appreciate.

[asbjornh]

Ah, that makes sense! :) Yeah, I'll just wait a bit and see if I get a confirmation from OP before I create the PR.

No worries; happy to contribute :)

[asbjornh]

@shaketbaby Doesn't look like I'll get any help testing (other than my own testing), so I created a PR now :)

[shaketbaby]

Thanks @asbjornh , the changes are backward compatible, so should be fine.

[shaketbaby]

v4.0.1 released with the fix.

[asbjornh]

Cool! :D that was quick. Thanks!