karma-runner / karma-commonjs

A Karma plugin. Test CommonJS modules.
MIT License
73 stars 30 forks source link

feat: support index.js for folder modules #22

Closed pkozlowski-opensource closed 10 years ago

pkozlowski-opensource commented 10 years ago

This is the first attempt at supporting more of the node's algorithm for modules resolving: http://nodejs.org/api/modules.html#modules_all_together

I'm starting with the support for index.js for folder modules as it allows me to do the basic code restructure that prepares support for other elements (see https://github.com/karma-runner/karma-commonjs/issues/21).

The idea here is that there is a function (getDependencyPathCandidates) that returns all candidate paths in order in which those paths should be tried. As soon as it is done we try the candidate paths in order and settle for the first module found.

pkozlowski-opensource commented 10 years ago

@vojtajina @jamesshore would be awesome to have your input for this change as it is the beginning of a bigger restructure I'm planning to do in the future.

jamesshore commented 10 years ago

@pkozlowski-opensource This looks like a good start to solving the module problem. I like that you have the tests in there now, too.

My only critique is that the code is still largely using the hacked-together design I initially created. It could benefit from some refactoring love. For example, normalizePath() could be a function called by getDependencyPathCandidates(). Similarly, the loop in require() could be factored out and simplified.

Here's an example of refactoring require(). It hasn't been tested or even linted, so use at your own discretion. :-)

var cachedModules = {}

validateGlobals();
loadAllModules();

function require(requiringFile, dependency) {
    var dependencyPath = findModulePath(requiringFile, dependency);
    var module = loadModule(dependencyPath);
    return module.exports;
}

function loadAllModules() {
    for (var modulePath in window.__cjs_module__) {
        require(modulePath, modulePath);
    }
}

function validateGlobals(requiringFile, dependency) {
    if (window.__cjs_module__ === undefined) throw new Error("Could not find any modules. Did you remember to set 'preprocessors' in your Karma config?");
    if (window.__cjs_modules_root__ === undefined) throw new Error("Could not find CommonJS module root path. Please report this issue to the karma-commonjs project.");
}   

function findModulePath(requiringFile, dependency) {
    var dependencyPaths = getDependencyPathCandidates(requiringFile, dependency, window.__cjs_modules_root__);
    for (var i = 0; i < dependencyPaths.length; i++) {
        var path = dependencyPaths[i];
        if (window.__cjs_module__[path] !== undefined) return path;
    }
  throw new Error("Could not find module '" + dependency + "' from '" + requiringFile + "'");
}

var loadModule(path) {
    var module = cachedModules[path];
    return module;

    module = { exports: {} };
    cachedModules[path] = module;

    moduleFn = window.__cjs_module__[path];
    moduleFn(requireFn(path), module, module.exports);

    return module;
}
pkozlowski-opensource commented 10 years ago

@jamesshore thnx for having a look. You are totally right that in this PR I don't do major rafactorings. The reason for taking baby-steps here is that:

But I'm totally with you that the current code should drastically change if we are support all the use-cases of the node's module load algorithm.

If you don't see major problems with the overall approach maybe let's merge this baby step and I will "go wild" when adding support for package.json? How does it sound?

jamesshore commented 10 years ago

@pkozlowski-opensource Totally fine with me, and I agree with your reasons. I don't see any problems with your basic approach.

pkozlowski-opensource commented 10 years ago

@jamesshore ok, merged this one as-is, going to do bigger refactoring while adding support for package.json.