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

Idea on package.json approach #7

Open
dead-claudia opened this issue Apr 29, 2016 · 2 comments
Open

Idea on package.json approach #7

dead-claudia opened this issue Apr 29, 2016 · 2 comments

Comments

@dead-claudia
Copy link

dead-claudia commented Apr 29, 2016

Edit: fix a couple small bugs.

I was thinking of an idea that might speed up the package.json approach, making it much quicker to check, which should keep the loading time minimal. The package.json will still need to be located, but that can be done in a similar search as what's done for node_modules now.

  • Make the package.json fields relevant for this (e.g. modules, modules.root) queryable in a cache trie, so it can be quickly tested, almost constant time in practice.
  • If no package.json was found in the above search, then the package.json should be found, and the file's parent directory added to the cache.

I've also prepared this gist to show what I mean better. One of those is a very high-level, unoptimized version, and the other is a more low-level, optimized equivalent that avoids allocation, although neither is tested.

With that gist, you would basically do this:

// In lib/module.js

const check = NativeModule.require("internal/check-path.js");
const path = NativeModule.require("path");
const hasOwn = Object.prototype.hasOwnProperty;

// ...

function loadPackageDirs(pkg, requestPath) {
  if (hasOwn.call(pkg, "modules")) {
    for (let i = 0; i < pkg.modules.length; i++) {
      check.addPath(path.resolve(pkg.modules[i]));
    }
  }

  if (hasOwn.call(pkg, "modules.root")) {
    check.addPath(path.resolve(pkg["modules.root"]));
  }

  if (hasOwn.call(pkg, "module")) {
    const dir = hasOwn.call(pkg, "main") ? pkg.module : requestPath;
    check.addPath(path.resolve(dir));
  }
}

function readPackage(requestPath) {
  // normal loading, declares `pkg`
  loadPackageDirs(pkg);
  return pkg;
}

function isModule(file) {
  // Check this first, since usually, the package has already been loaded. It's a
  // cheap check, anyways.
  if (check.hasPath(file)) {
    return true;
  }

  let last = path.dirname(file)
  let current;

  // The dirname of the root is the root.
  while (current !== last) {
    if (!hasOwn.call(packageCache, current)) {
      if (fs.statSync(path.join(current, "package.json")).isDirectory()) {
        readPackage(current);
        break;
      }
    }

    current = last;
    last = path.dirname(current);
  }

  // This is only true if a package.json has been loaded
  if (check.hasPath(file)) {
    return true;
  }

  return path.basename(file).startsWith("module.");
}
@caridy
Copy link
Collaborator

caridy commented Apr 29, 2016

@isiahmeadows thanks for putting this together. we know the lookup process in node will not be a problem and will not add a significant perf penalty, but it is nice to have your code around in case they ask for prove :)

@dead-claudia
Copy link
Author

dead-claudia commented Apr 29, 2016

@caridy Welcome 😄

Another thing about doing it that way is that the memory is likely going to be much smaller than the naïve implementation. V8 stores strings as persistent cons cells IIRC, and smaller strings means they're more likely to be reused internally within larger strings. If I'm wrong about that, then the memory gains will be even larger.

For what it's worth, I've done similar in my WIP test framework. The only difference is that I had to use arrays for the children because I needed to match against regexps as well.

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