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

Rewrite path parsing using the path module instead of regexes #14

Open
sorgloomer opened this issue Aug 22, 2016 · 6 comments
Open

Rewrite path parsing using the path module instead of regexes #14

sorgloomer opened this issue Aug 22, 2016 · 6 comments

Comments

@sorgloomer
Copy link

In the recent weeks we had multiple high priority bugs, which were caused by the same reason: parsing paths with regexes.

I happily create a PR for this, but I have no idea what does the getModuleName function do. Are there some test cases for that?

@developit
Copy link
Owner

I don't believe there are tests for it. getModuleName() accepts a file path (a package main) and attempts to pluck out the NPM package name from it. Basically the next directory within the deepest node_modules directory. There is a slight twist in that namespaced modules consist of two path segments. I don't mind writing some tests for the function, or refactoring it to split on path.separator and inspect the parts.

@sorgloomer
Copy link
Author

It turns out nodejs still does not have path.split, we cant bypass regexes entirely

@sorgloomer
Copy link
Author

sorgloomer commented Aug 22, 2016

We could use something like this, but I don't particularly like the idea of a loop that I can't prove it will terminate.

var path = require("path");
function pathSplit(pathname) {
  var parts = [];
  for (;;) {
    var basename = path.basename(pathname);
    var dirname = path.dirname(pathname);
    if (dirname === pathname) {
      break;
    }
    pathname = dirname;
    parts.push(basename);
  }
  parts.reverse();
  return parts;
}

console.log(pathSplit("a/b/sdfsdf//cccv/node_modules/dsgdgfh")); // [ 'a', 'b', 'sdfsdf', 'cccv', 'node_modules', 'dsgdgfh' ]

@sorgloomer
Copy link
Author

sorgloomer commented Aug 22, 2016

This looks better, only that it still doesn't use the path module:

function getModuleName(pathname) {
  var parts = pathname.split(/[\\/]/g).filter(p => p !== "" && p !== "." && p !== "..");
  if (parts.length <= 0) {
    return null;
  }
  var nodeModulesIdx = parts.lastIndexOf("node_modules");
  if (nodeModulesIdx < 0 || nodeModulesIdx + 1 >= parts.length) {
    return parts[parts.length - 1];
  }
  return parts[nodeModulesIdx + 1];
}

console.log(getModuleName("../../babel-plugin-asd")); // babel-plugin-asd
console.log(getModuleName("../..//babel-plugin-asd")); // babel-plugin-asd
console.log(getModuleName("..//../babel-plugin-asd")); // babel-plugin-asd
console.log(getModuleName("c:\\node_modules/babel-plugin-asd")); // babel-plugin-asd
console.log(getModuleName("../node_modules/xxx/../node_modules///babel-plugin-asd/xyz")); // babel-plugin-asd
console.log(getModuleName("xxx/../node_modules/..//babel-plugin-asd/xyz")); // babel-plugin-asd

What do you think?

@developit
Copy link
Owner

@sorgloomer Needs some extra logic to support namespaced packages. @foo/bar - they show up as directories within node_modules.

@developit
Copy link
Owner

Also, it might be easier to use path.resolve(pathname) so we don't have to worry about jumps? For the split(), could do: pathname.split(path.separator).

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

2 participants