fix relative module import (#83)

* Regression test * Fixed issue where relative paths where not correctly refering their parent * Made module parent name optional
hostnet · Jul 12, 2018 · d167bcc · d167bcc
1 parent a4159f6
commit d167bcc
Show file tree

Hide file tree

Showing 11 changed files with 131 additions and 30 deletions.
diff --git a/spec/require.spec.js b/spec/require.spec.js
@@ -122,14 +122,14 @@ describe("Require.js module register method test", function () {
     });
 
     it("with CommonJS exports", function() {
-        lib.register("commonjs_exports/foo", function (define, require, module, exports) {
+        lib.register("commonjs_exports/foo", "commonjs_exports", function(define, require, module, exports) {
             define([], function () {
                 return 'commonjs_exports/foo';
             });
         });
 
 
-        lib.register("commonjs_exports/bar", function (define, require, module, exports) {
+        lib.register("commonjs_exports/bar", "commonjs_exports", function(define, require, module, exports) {
             var bar = require('./foo');
 
             exports.Foo = true;
@@ -147,10 +147,10 @@ describe("Require.js module register method test", function () {
     });
 
     it("with relative require paths", function() {
-        lib.register("this/is/relative/path", function (define, require, module, exports) {
+        lib.register("this/is/relative/path", "this/is/relative", function(define, require, module, exports) {
             return 'HENK';
         });
-        lib.register("this/is/spartha", function (define, require, module, exports) {
+        lib.register("this/is/spartha", "this/is", function(define, require, module, exports) {
             return require('./relative/path');
         });
 
@@ -200,6 +200,21 @@ describe("Require.js module register method test", function () {
         expect(lib.require('pathdeps/pathdepsbar')).toEqual('BAR');
     });
 
+    it("with relative paths", function() {
+        lib.register("relfoo/bar", function(define, require, module, exports) {
+            define([], function () {
+                return 'BAR';
+            });
+        });
+        lib.register("relfoo", function(define, require, module, exports) {
+            define([], function () {
+                return require('./bar');
+            });
+        });
+
+        expect(lib.require('relfoo')).toEqual('BAR');
+    });
+
     it("with unknown require", function() {
         try {
             lib.require('somemodule');

diff --git a/spec/steps/module.spec.js b/spec/steps/module.spec.js
@@ -3,12 +3,23 @@ let builder = require('../../src/Builder/js/build');
 describe("module.js", function () {
     it('execute', function () {
         let step = require('../../src/Builder/js/steps/module');
-        let result = step(new builder.File('foo.js', 'foo.js', Buffer.from("foobar();")));
+        let result = step(new builder.File('foo.js', 'foo.js', Buffer.from("foobar();"), '', 'foo'));
 
         expect(result.name).toBe('foo.js');
         expect(result.module).toBe('foo.js');
         expect(result.content.toString()).toBe(
-            'register("foo.js", function (define, require, module, exports) {\nfoobar();\n});\n'
+            'register("foo.js", "foo", function (define, require, module, exports) {\nfoobar();\n});\n'
+        );
+    });
+
+    it('execute same as parent', function () {
+        let step = require('../../src/Builder/js/steps/module');
+        let result = step(new builder.File('foo.js', 'foo/bar', Buffer.from("foobar();"), '', 'foo/bar'));
+
+        expect(result.name).toBe('foo.js');
+        expect(result.module).toBe('foo/bar');
+        expect(result.content.toString()).toBe(
+            'register("foo/bar", function (define, require, module, exports) {\nfoobar();\n});\n'
         );
     });
 });
diff --git a/src/Builder/BuildFiles.php b/src/Builder/BuildFiles.php
@@ -12,6 +12,7 @@
 use Hostnet\Component\Resolver\Import\Dependency;
 use Hostnet\Component\Resolver\Import\DependencyNodeInterface;
 use Hostnet\Component\Resolver\Import\ImportFinderInterface;
+use Hostnet\Component\Resolver\Module;
 use Hostnet\Component\Resolver\Report\ReporterInterface;
 use Symfony\Component\Filesystem\Filesystem;
 
@@ -144,12 +145,15 @@ private function addToFiles(File $base_file, array $dependencies, bool $skip_fil
                 $module_name = $base_dir . $file->getBaseName() . '.' . $file->extension;
             }
 
+            $parent_module = \dirname($module_name);
+
             $this->files[$output_file->getName()][] = [
                 $file->path,
                 '.' . $file->extension,
                 $module_name,
                 $force || $mtime === -1 || $this->hasUpdatedFiles($mtime, $dep),
                 $skip_file_actions,
+                $file instanceof Module ? $file->getParentName() : ($parent_module === '.' ? '' : $parent_module),
             ];
         }
     }

diff --git a/src/Builder/js/build.js b/src/Builder/js/build.js
@@ -1,20 +1,22 @@
 let path = require('path'), fs = require('fs'), crypto = require('crypto');
 
-let BuildableFile = function (filePath, extension, moduleName, needsRebuild, skipFileSteps) {
+let BuildableFile = function (filePath, extension, moduleName, needsRebuild, skipFileSteps, parentPath) {
     this.path = filePath;
     this.extension = extension !== undefined ? extension : path.extname(filePath);
     this.moduleName = moduleName !== undefined ? moduleName : filePath;
     this.needsRebuild = needsRebuild !== undefined ? needsRebuild : true;
     this.skipFileSteps = skipFileSteps !== undefined ? skipFileSteps : false;
+    this.parentPath = parentPath !== undefined ? parentPath : path.dirname(filePath);
 };
 
 BuildableFile.fromData = function (data) {
-    return new BuildableFile(data[0], data[1], data[2], data[3], data[4])
+    return new BuildableFile(data[0], data[1], data[2], data[3], data[4], data[5])
 };
 
-let File = function (name, module, content, outputFile) {
+let File = function (name, module, content, outputFile, parentPath) {
     this.name = name;
     this.outputFile = outputFile;
+    this.parentPath = parentPath;
     this.additionalFiles = [];
 
     this.update = function (newContent, moduleName) {
@@ -44,7 +46,7 @@ let File = function (name, module, content, outputFile) {
 };
 
 File.fromBuildFile = function (buildFile, content) {
-    return new File(buildFile.path, buildFile.moduleName, content, buildFile.moduleName);
+    return new File(buildFile.path, buildFile.moduleName, content, buildFile.moduleName, buildFile.parentPath);
 };
 
 function mkdirRecursive(rootDir, pathToCreate) {
@@ -161,11 +163,7 @@ function compileFile(buildableFile, config, logger) {
                         );
                     }
 
-                    resolve(require(steps[j])(
-                        file,
-                        config,
-                        (file) => compileFile(new BuildableFile(file), config, logger)
-                    ));
+                    resolve(require(steps[j])(file, config, (file) => compileFile(new BuildableFile(file), config, logger)));
                 } catch (err) {
                     reject(err);
                 }

diff --git a/src/Builder/js/require.js b/src/Builder/js/require.js
@@ -20,7 +20,12 @@
         var require_name = name;
 
         if (require_name.indexOf('.') === 0) {
-            var parent = parent_module.substring(0, parent_module.lastIndexOf('/'));
+            var parent;
+            if (_modules[parent_module]) {
+                parent = _modules[parent_module]._parent;
+            } else {
+                parent = parent_module.substring(0, parent_module.lastIndexOf('/'));
+            }
 
             if (require_name.indexOf('./') === 0) {
                 require_name = require_name.substr(2);
@@ -125,16 +130,23 @@
         return _modules[name]._module;
     };
 
-    window.register = function (name, initializer) {
+    window.register = function (name, parent, initializer) {
         if (_modules[name]) {
             if (typeof console !== 'undefined' && typeof console.warn === 'function') {
                 console.warn(new ModuleRedeclareError(name));
             }
             return;
         }
 
+        // special case where only two arguments were given: register("jquery", function (...) {})
+        if (typeof initializer === "undefined" && typeof parent === 'function') {
+            initializer = parent;
+            parent = name;
+        }
+
         _modules[name] = {
             _initializer: initializer,
+            _parent: parent,
             _module: null
         };
     };

diff --git a/src/Builder/js/steps/module.js b/src/Builder/js/steps/module.js
@@ -1,3 +1,11 @@
 module.exports = function (file) {
-    return file.update(Buffer.from("register(" + JSON.stringify(file.module) + ", function (define, require, module, exports) {\n" + file.content + "\n});\n"));
+    let args = JSON.stringify(file.module);
+
+    if (file.module !== file.parentPath) {
+        args += ", " + JSON.stringify(file.parentPath);
+    }
+
+    let content = "register(" + args + ", function (define, require, module, exports) {\n" + file.content + "\n});\n";
+
+    return file.update(Buffer.from(content));
 };
diff --git a/src/Module.php b/src/Module.php
@@ -34,6 +34,22 @@ public function getName(): string
      */
     public function getParentName(): string
     {
-        return \dirname($this->name);
+        $dir_parts = explode('/', $this->path);
+        $module_parts = explode('/', $this->name);
+        $parts = [];
+
+        for ($i = 0, $n = \count($dir_parts); $i < $n; $i++) {
+            if ($dir_parts[$i] === $module_parts[0]) {
+                break;
+            }
+
+            $parts[] = $dir_parts[$i];
+        }
+
+        if (empty($parts)) {
+            return '';
+        }
+
+        return substr(\dirname($this->path), \strlen(implode('/', $parts)) + 1) ?: $this->name;
     }
 }
diff --git a/test/Builder/BuildFilesTest.php b/test/Builder/BuildFilesTest.php
@@ -85,6 +85,7 @@ public function testCompile(): void
                         '../../src/Builder/js/require.js',
                         true,
                         true,
+                        '../../src/Builder/js',
                     ],
                 ],
                 'dist/fixtures/foo.js' => [
@@ -94,6 +95,7 @@ public function testCompile(): void
                         'foo.js',
                         true,
                         false,
+                        '',
                     ],
                 ],
                 'dist/sub/bar.js' => [
@@ -103,6 +105,7 @@ public function testCompile(): void
                         'sub/bar.js',
                         true,
                         false,
+                        'sub',
                     ],
                 ],
             ], $data['input']);
@@ -186,13 +189,15 @@ public function testCompileWithOutdatedCacheAndOutput(): void
                         'foo.js',
                         false,
                         false,
+                        '',
                     ],
                     [
                         'fixtures/sub/bar.js',
                         '.js',
                         'sub/bar.js',
                         false,
                         false,
+                        'sub',
                     ],
                 ],
             ], $data['input']);

diff --git a/test/Builder/stdin.expected.json b/test/Builder/stdin.expected.json
@@ -6,7 +6,8 @@
                 ".js",
                 "..\/..\/src\/Builder\/js\/require.js",
                 true,
-                true
+                true,
+                "..\/..\/src\/Builder\/js"
             ]
         ],
         "dist\/fixtures\/foo.js": [
@@ -15,7 +16,8 @@
                 ".js",
                 "foo.js",
                 true,
-                false
+                false,
+                ""
             ]
         ],
         "dist\/bar.js": [
@@ -24,8 +26,9 @@
                 ".js",
                 "bar.js",
                 true,
-                false
+                false,
+                ""
             ]
         ]
     }
-}
+}
diff --git a/test/Import/Nodejs/FileResolverTest.php b/test/Import/Nodejs/FileResolverTest.php
@@ -149,6 +149,16 @@ public function testAsRequireModuleFromOtherPath()
         self::assertSame('uikit', $import->getImportedFile()->getName());
     }
 
+    public function testAsRequireModuleRelativeFromDifferentPath()
+    {
+        $parent = new Module('module_package', 'node_modules/module_package/main.js');
+        $import = $this->file_resolver->asRequire('./subpackage/main', $parent);
+
+        self::assertInstanceOf(Module::class, $import->getImportedFile());
+        self::assertSame('node_modules/module_package/subpackage/main.js', $import->getImportedFile()->path);
+        self::assertSame('module_package/subpackage/main', $import->getImportedFile()->getName());
+    }
+
     /**
      * @expectedException \Hostnet\Component\Resolver\Import\Nodejs\Exception\FileNotFoundException
      */

diff --git a/test/ModuleTest.php b/test/ModuleTest.php
@@ -15,15 +15,34 @@ class ModuleTest extends TestCase
 {
     public function testGeneric()
     {
-        $file = new Module('foo/bar', __FILE__);
+        $file = new Module('foo/bar', 'node_modules/foo/bar/index.js');
 
-        self::assertEquals(__DIR__, $file->dir);
-        self::assertEquals('php', $file->extension);
+        self::assertEquals('node_modules/foo/bar', $file->dir);
+        self::assertEquals('js', $file->extension);
         self::assertEquals('foo/bar', $file->getName());
-        self::assertEquals('foo', $file->getParentName());
-        self::assertEquals('.', (new Module('foo', __FILE__))->getParentName());
-        self::assertEquals('ModuleTest', $file->getBaseName());
-        self::assertTrue($file->equals(new File(__FILE__)));
+        self::assertEquals('foo/bar', $file->getParentName());
+        self::assertEquals('index', $file->getBaseName());
+        self::assertTrue($file->equals(new File('node_modules/foo/bar/index.js')));
         self::assertFalse($file->equals(new File('someOtherFile.php')));
     }
+
+    /**
+     * @dataProvider getParentNameProvider
+     */
+    public function testGetParentName(string $expected, string $name, string $path): void
+    {
+        self::assertEquals($expected, (new Module($name, $path))->getParentName());
+    }
+
+    public function getParentNameProvider(): array
+    {
+        return [
+            ['', 'foo', 'foo/index.js'],
+            ['', 'bar.js', 'bar.js'],
+            ['foo/bar', 'foo/bar', 'node_modules/foo/bar/index.js'],
+            ['foo', 'foo', 'node_modules/foo/index.js'],
+            ['foo/dist', 'foo', 'node_modules/foo/dist/index.js'],
+            ['rxjs/internal', 'rxjs/internal/Rx', 'node_modules/rxjs/internal/Rx.js'],
+        ];
+    }
 }