[JENKINS-37714] Take dedupe into account when translating fullPaths t…

…o Ids (#8) * [FIX JENKINS-37714] Take dedupe into account when translating fullPaths to Ids * 0.0.38-beta2 * Search for the list of modules to be imported using a rouch string search Vs using a require transform and explicitly searching the require statements, which can fail ala benbria/browserify-transform-tools#23 * 0.0.38-beta3 * 0.0.38
jenkinsci · Aug 29, 2016 · b2e6eed · b2e6eed
1 parent 2bb5202
commit b2e6eed
Show file tree

Hide file tree

Showing 7 changed files with 69 additions and 9 deletions.
diff --git a/internal/bundlegen.js b/internal/bundlegen.js
@@ -274,23 +274,27 @@ function addModuleMappingTransforms(bundle, bundler) {
     var requiredModuleMappings = [];
 
     if (moduleMappings.length > 0) {
-        var requireTransform = transformTools.makeRequireTransform("requireTransform",
-            {evaluateArguments: true},
-            function(args, opts, cb) {
-                var required = args[0];
+        var requireSearch = transformTools.makeStringTransform("requireSearch", {},
+            function(content, opts, cb) {
                 for (var i = 0; i < moduleMappings.length; i++) {
                     var mapping = moduleMappings[i];
-                    if (mapping.fromSpec.moduleName === required) {
+                    // Do a rough search for the module name. If we find it, then we
+                    // add that module name to the list. This may result in some false
+                    // positives, but that's okay. The most important thing is that we
+                    // do add the import for the module if it is required. Adding additional
+                    // imports for modules not required is not optimal, but is also not the
+                    // end of the world.
+                    if (content.indexOf(mapping.fromSpec.moduleName) !== -1) {
                         var toSpec = new ModuleSpec(mapping.to);
                         var importAs = toSpec.importAs();
                         if (requiredModuleMappings.indexOf(importAs) === -1) {
                             requiredModuleMappings.push(importAs);
                         }
                     }
                 }
-                return cb();
+                return cb(null, content);
             });
-        bundler.transform({ global: true }, requireTransform);
+        bundler.transform({ global: true }, requireSearch);
     }
     var importExportApplied = false;
     var importExportTransform = transformTools.makeStringTransform("importExportTransform", {},

diff --git a/internal/pipeline-transforms/require-stub-transform.js b/internal/pipeline-transforms/require-stub-transform.js
@@ -322,6 +322,9 @@ function fullPathsToIds(metadata) {
     }
 
     function mapDependencyId(from, to) {
+        var dedupeSourceFrom = 'arguments[4]["' + from + '"][0].apply(exports,arguments)';
+        var dedupeSourceTo = 'arguments[4][' + to + '][0].apply(exports,arguments)';
+
         for (var i in metadata.packEntries) {
             if (metadata.packEntries.hasOwnProperty(i)) {
                 var packEntry = metadata.packEntries[i];
@@ -332,6 +335,12 @@ function fullPathsToIds(metadata) {
                         packDeps[dep] = to;
                     }
                 }
+
+                // Check the pack entry source for it being a dedupe,
+                // translating it if required.
+                if (packEntry.source === dedupeSourceFrom) {
+                    packEntry.source = dedupeSourceTo;
+                }
             }
         }
     }

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@jenkins-cd/js-builder",
-  "version": "0.0.37",
+  "version": "0.0.38",
   "description": "Jenkins CI JavaScript Build utilities.",
   "main": "index.js",
   "scripts": {

diff --git a/spec/modules/dedupe-main.js b/spec/modules/dedupe-main.js
@@ -0,0 +1,8 @@
+
+// Require two different modules having identical content. This
+// should cause browserify to dedupe the entries for both of these. 
+var d1 = require('./dedupe-one');
+var d2 = require('./dedupe-two');
+
+console.log(d1);
+console.log(d2);
diff --git a/spec/modules/dedupe-one.js b/spec/modules/dedupe-one.js
@@ -0,0 +1,5 @@
+// A file with nothing in it ... dedupe-one and dedupe-two must have identical content
+
+var module = require('./module4');
+
+console.log('*** module: ', module);
diff --git a/spec/modules/dedupe-two.js b/spec/modules/dedupe-two.js
@@ -0,0 +1,5 @@
+// A file with nothing in it ... dedupe-one and dedupe-two must have identical content
+
+var module = require('./module4');
+
+console.log('*** module: ', module);
diff --git a/spec/require-stub-transform-spec.js b/spec/require-stub-transform-spec.js
@@ -139,6 +139,28 @@ describe("require-stub-transform", function () {
         });
     });
 
+
+    it("- test browserify dedupe - JENKINS-37714", function (done) {
+        buildBrowserPack('dedupe-main.js', function (packEntries) {
+                        var metadata = transformModule.updateBundleStubs(packEntries, []);
+
+            // We should have defs for all modules except module4, module5 and module6.
+            var dedupeOnePackEntry = getPackEntryByName(metadata, './dedupe-one');
+            var dedupeTwoPackEntry = getPackEntryByName(metadata, './dedupe-two');
+
+            expect(dedupeOnePackEntry).toBeDefined();
+            expect(dedupeTwoPackEntry).toBeDefined();
+            expect(dedupeOnePackEntry.id).toBe(2);
+            expect(dedupeTwoPackEntry.id).toBe(3);
+
+            // The contents of both these modules are identical, causing browserify
+            // to optimize by pointing dedupeTwoPackEntry to just point to dedupeOnePackEntry.
+            // We ant to check that the module id was properly translated.
+            expect(dedupeTwoPackEntry.source).toBe('arguments[4][' + dedupeOnePackEntry.id + '][0].apply(exports,arguments)');
+
+            done()
+        });
+    });    
 });
 
 function getPackEntryByName(metadata, name) {
@@ -173,7 +195,14 @@ function logMetadata(metadata) {
 }
 
 function buildBrowserPack(source, onDone) {
-    var b = browserify('./spec/modules/' + source);
+    var browserifyConfig = {
+        entries: ['./spec/modules/' + source],
+        extensions: ['.js'],
+        cache: {},
+        packageCache: {},
+        fullPaths: true
+    };
+    var b = browserify(browserifyConfig);
 
     if (!fs.existsSync('./target/testbundles')) {
         fs.mkdirSync('./target/testbundles');