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

Using a windows path in the alias replace string breaks #242

Open
vjpr opened this issue Dec 6, 2017 · 8 comments
Open

Using a windows path in the alias replace string breaks #242

vjpr opened this issue Dec 6, 2017 · 8 comments

Comments

@vjpr
Copy link

vjpr commented Dec 6, 2017

When wallaby.projectCacheDir is C:\x\y\.z\system\wallaby\projects\6efd40f7185030b1...

require('packages/foo') tries to look for C:\x\y\.z\system\wallaby\projectsefd40f7185030b1\instrumented/packages/foo.

The windows path backslashes are escaped, and then it matches \\6 after projects.

module.exports = function moduleResolverPlugin(wallaby) {
  return [
    'module-resolver',
    {
      alias: {
        '^@test/(.+)': wallaby.projectCacheDir + path.sep + 'packages/\\1',
        '^packages/(.+)': wallaby.projectCacheDir + path.sep + '',
      },
    },
  ]
}

You can reference the n-th matched group with '\\n' ('\\0' refers to the whole matched path).

To use the backslash character (\) just escape it like so: '\\\\' (double escape is needed because of JSON already using \ for escaping).

Using \\n for match groups has the potential for really weird issues. Can we change it to something else?

@fatfisz
Copy link
Contributor

fatfisz commented Dec 7, 2017

Hmm we definitely couldn't change that in a short time - this is a breaking change. It's also impossible to infer what is the meaning of \6 from your example without some funny heuristics in the plugin.

I'm not convinced that complicating the RegExp alias config is a good trade-off for handling backslashes from Windows paths. As someone who works on Windows myself, I care about properly handling paths in different systems a lot, but in this case we're in the realm of JS regular expressions. Any solution in the plugin other than the current would be hackish with respect to the regular expressions themselves.

I'd suggest replacing backslashes with slashes here: as a user of the plugin you have much more control over what to do with the data that is passed. I feel your pain, but I'm skeptical we'd find a better solution here. Unless you could present some ideas?

@vjpr
Copy link
Author

vjpr commented Dec 8, 2017

Hmm we definitely couldn't change that in a short time - this is a breaking change

What about allowing the following (would not be breaking):

module.exports = function moduleResolverPlugin(wallaby) {
  return [
    'module-resolver',
    {
      alias: {
        '^@test/(.+)': {replace: 'foo\\654\\\1'},
        '^packages/(.+)': {replace: 'foo\\654\\\1'},
      },
    },
  ]
}
``` @

@tleunen
Copy link
Owner

tleunen commented Dec 8, 2017

Hmm.. Yeah.. this sucks for windows :/ And it would definitely be great to support Wallaby.
Maybe we could use a custom syntax instead of the usual \\n to prevent any issue using a path delimiter.

@fatfisz
Copy link
Contributor

fatfisz commented Dec 9, 2017

Here's an idea: since I see that you're using JS to configure Babel, would allowing a function as a value of the alias be acceptable for you? E.g.

module.exports = function moduleResolverPlugin(wallaby) {
  return [
    'module-resolver',
    {
      alias: {
        '^@test/(.+)': ([, name]) => path.join(wallaby.projectCacheDir, 'packages', name),
      },
    },
  ]
}

I think this looks clean enough and should solve your problem.

@fatfisz
Copy link
Contributor

fatfisz commented Dec 9, 2017

The argument would be the result of calling exec on a matched path (so it would never be null). This means that the argument is always an array with the full match and the matched groups, which can be used as desired.

@vjpr
Copy link
Author

vjpr commented Dec 9, 2017

@fatfisz Yep, this is fine.

@tleunen
Copy link
Owner

tleunen commented Dec 12, 2017

@fatfisz Another solution would also be to replace the \\n from the regexp into $n.
That's what jest uses with their moduleNameMapper option for example.

@fatfisz
Copy link
Contributor

fatfisz commented Dec 12, 2017

Hmm, this would require a special treatment of the native \n groups... not convinced it's the best idea (though I'm not ruling it out, either).

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

No branches or pull requests

3 participants