Fix npm file references package lock (#304)

* Rebase package-lock

* Fix rebasePackageLock call

* Adding tests for package-lock file rewrite

* Fix comment

Author: franciscocpg

lib/packExternalModules.js

@@ -7,6 +7,27 @@ const childProcess = require('child_process');
 const fse = require('fs-extra');
 const isBuiltinModule = require('is-builtin-module');
 
+function rebaseFileReferences(pathToPackageRoot, moduleVersion) {
+  if (/^file:[^/]{2}/.test(moduleVersion)) {
+    const filePath = _.replace(moduleVersion, /^file:/, '');
+    return _.replace(`file:${pathToPackageRoot}/${filePath}`, /\\/g, '/');
+  }
+
+  return moduleVersion;
+}
+
+function rebasePackageLock(pathToPackageRoot, module) {
+  if (module.version) {
+    module.version = rebaseFileReferences(pathToPackageRoot, module.version);
+  }
+
+  if (module.dependencies) {
+    _.forIn(module.dependencies, moduleDependency => {
+      rebasePackageLock(pathToPackageRoot, moduleDependency);
+    });
+  }
+}
+
 /**
  * Add the given modules to a package json's dependencies.
  */
@@ -20,10 +41,7 @@ function addModulesToPackageJson(externalModules, packageJson, pathToPackageRoot
     }
     let moduleVersion = _.join(_.tail(splitModule), '@');
     // We have to rebase file references to the target package.json
-    if (/^file:[^/]{2}/.test(moduleVersion)) {
-      const filePath = _.replace(moduleVersion, /^file:/, '');
-      moduleVersion = _.replace(`file:${pathToPackageRoot}/${filePath}`, /\\/g, '/');
-    }
+    moduleVersion = rebaseFileReferences(pathToPackageRoot, moduleVersion);
     packageJson.dependencies = packageJson.dependencies || {};
     packageJson.dependencies[_.first(splitModule)] = moduleVersion;
   });
@@ -262,8 +280,21 @@ module.exports = {
       .then(exists => {
         if (exists) {
           this.serverless.cli.log('Package lock found - Using locked versions');
-          return BbPromise.fromCallback(cb => fse.copy(packageLockPath, path.join(compositeModulePath, 'package-lock.json'), cb))
-          .catch(err => this.serverless.cli.log(`Warning: Could not copy lock file: ${err.message}`));
+          try {
+            const packageLockJson = this.serverless.utils.readFileSync(packageLockPath);
+            /**
+             * We should not be modifying 'package-lock.json'
+             * because this file should be treat as internal to npm.
+             * 
+             * Rebase package-lock is a temporary workaround and must be
+             * removed as soon as https://github.com/npm/npm/issues/19183 gets fixed.
+             */
+            rebasePackageLock(relPath, packageLockJson);
+
+            this.serverless.utils.writeFileSync(path.join(compositeModulePath, 'package-lock.json'), JSON.stringify(packageLockJson, null, 2));
+          } catch(err) {
+            this.serverless.cli.log(`Warning: Could not read lock file: ${err.message}`);
+          }
         }
         return BbPromise.resolve();
       })

tests/packExternalModules.test.js

@@ -30,6 +30,7 @@ describe('packExternalModules', () => {
   let fsExtraMock;
   // Serverless stubs
   let writeFileSyncStub;
+  let readFileSyncStub;
 
   before(() => {
     sandbox = sinon.sandbox.create();
@@ -60,6 +61,7 @@ describe('packExternalModules', () => {
     _.set(serverless, 'service.service', 'test-service');
 
     writeFileSyncStub = sandbox.stub(serverless.utils, 'writeFileSync');
+    readFileSyncStub = sandbox.stub(serverless.utils, 'readFileSync');
     _.set(serverless, 'service.custom.webpackIncludeModules', true);
 
     module = _.assign({
@@ -73,6 +75,7 @@ describe('packExternalModules', () => {
   afterEach(() => {
     // Reset all counters and restore all stubbed functions
     writeFileSyncStub.reset();
+    readFileSyncStub.reset();
     childProcessMock.exec.reset();
     fsExtraMock.pathExists.reset();
     fsExtraMock.copy.reset();
@@ -258,6 +261,7 @@ describe('packExternalModules', () => {
     });
 
     it('should rebase file references', () => {
+      const expectedLocalModule = 'file:../../locals/../../mymodule';
       const expectedCompositePackageJSON = {
         name: 'test-service',
         version: '1.0.0',
@@ -274,14 +278,52 @@ describe('packExternalModules', () => {
         dependencies: {
           '@scoped/vendor': '1.0.0',
           uuid: '^5.4.1',
-          localmodule: 'file:../../locals/../../mymodule',
+          localmodule: expectedLocalModule,
           bluebird: '^3.4.0'
         }
       };
 
+      const fakePackageLockJSON = {
+        name: 'test-service',
+        version: '1.0.0',
+        description: 'Packaged externals for test-service',
+        private: true,
+        dependencies: {
+          '@scoped/vendor': '1.0.0',
+          uuid: {
+            version: '^5.4.1'
+          },
+          bluebird: {
+            version: '^3.4.0'
+          },
+          localmodule: {
+            version: 'file:../../mymodule'
+          }
+        }
+      };
+      const expectedPackageLockJSON = {
+        name: 'test-service',
+        version: '1.0.0',
+        description: 'Packaged externals for test-service',
+        private: true,
+        dependencies: {
+          '@scoped/vendor': '1.0.0',
+          uuid: {
+            version: '^5.4.1'
+          },
+          bluebird: {
+            version: '^3.4.0'
+          },
+          localmodule: {
+            version: expectedLocalModule
+          }
+        }
+      };
+
       _.set(serverless, 'service.custom.webpackIncludeModules.packagePath', path.join('locals', 'package.json'));
       module.webpackOutputPath = 'outputPath';
-      fsExtraMock.pathExists.yields(null, false);
+      readFileSyncStub.returns(fakePackageLockJSON);
+      fsExtraMock.pathExists.yields(null, true);
       fsExtraMock.copy.yields();
       childProcessMock.exec.onFirstCall().yields(null, '{}', '');
       childProcessMock.exec.onSecondCall().yields(null, '', '');
@@ -294,9 +336,10 @@ describe('packExternalModules', () => {
       return expect(module.packExternalModules()).to.be.fulfilled
       .then(() => BbPromise.all([
         // The module package JSON and the composite one should have been stored
-        expect(writeFileSyncStub).to.have.been.calledTwice,
+        expect(writeFileSyncStub).to.have.been.calledThrice,
         expect(writeFileSyncStub.firstCall.args[1]).to.equal(JSON.stringify(expectedCompositePackageJSON, null, 2)),
-        expect(writeFileSyncStub.secondCall.args[1]).to.equal(JSON.stringify(expectedPackageJSON, null, 2)),
+        expect(writeFileSyncStub.secondCall.args[1]).to.equal(JSON.stringify(expectedPackageLockJSON, null, 2)),
+        expect(writeFileSyncStub.thirdCall.args[1]).to.equal(JSON.stringify(expectedPackageJSON, null, 2)),
         // The modules should have been copied
         expect(fsExtraMock.copy).to.have.been.calledOnce,
         // npm ls and npm prune should have been called
@@ -702,10 +745,7 @@ describe('packExternalModules', () => {
         expect(writeFileSyncStub.firstCall.args[1]).to.equal(JSON.stringify(expectedCompositePackageJSON, null, 2)),
         expect(writeFileSyncStub.secondCall.args[1]).to.equal(JSON.stringify(expectedPackageJSON, null, 2)),
         // The modules should have been copied
-        expect(fsExtraMock.copy).to.have.been.calledTwice,
-        expect(fsExtraMock.copy.firstCall).to.have.been.calledWith(
-          sinon.match(/package-lock.json$/)
-        ),
+        expect(fsExtraMock.copy).to.have.been.calledOnce,
         // npm ls and npm prune should have been called
         expect(childProcessMock.exec).to.have.been.calledThrice,
         expect(childProcessMock.exec.firstCall).to.have.been.calledWith(
@@ -741,9 +781,9 @@ describe('packExternalModules', () => {
       };
 
       module.webpackOutputPath = 'outputPath';
+      readFileSyncStub.throws(new Error('Failed to read package-lock.json'));
       fsExtraMock.pathExists.yields(null, true);
-      fsExtraMock.copy.onFirstCall().yields(new Error('Failed to read package-lock.json'));
-      fsExtraMock.copy.onSecondCall().yields();
+      fsExtraMock.copy.onFirstCall().yields();
       childProcessMock.exec.onFirstCall().yields(null, '{}', '');
       childProcessMock.exec.onSecondCall().yields(null, '', '');
       childProcessMock.exec.onThirdCall().yields();
@@ -755,10 +795,7 @@ describe('packExternalModules', () => {
         expect(writeFileSyncStub.firstCall.args[1]).to.equal(JSON.stringify(expectedCompositePackageJSON, null, 2)),
         expect(writeFileSyncStub.secondCall.args[1]).to.equal(JSON.stringify(expectedPackageJSON, null, 2)),
         // The modules should have been copied
-        expect(fsExtraMock.copy).to.have.been.calledTwice,
-        expect(fsExtraMock.copy.firstCall).to.have.been.calledWith(
-          sinon.match(/package-lock.json$/)
-        ),
+        expect(fsExtraMock.copy).to.have.been.calledOnce,
         // npm ls and npm prune should have been called
         expect(childProcessMock.exec).to.have.been.calledThrice,
         expect(childProcessMock.exec.firstCall).to.have.been.calledWith(