Repository: serverless-heaven/serverless-webpack (js)

Issue:
Start offline after webpack is compiled
# This is a Bug Report 

## Description

* What went wrong?
Building of webpack is async and is often completed after `offline` has started. This is limiting as the `--exec` parameter queues a script to start firing requests against the application which is not yet built.

* What did you expect should have happened?

1. sls offline start
2. start webpack build
3. end webpack build
4. start offline plugin
5. exec script
6. stop offline plugin

* What was the config you used
`./node_modules/serverless/bin/serverless offline -c start --exec \"yarn test-system\"

* What stacktrace or error message from your provider did you see?
n/a

## Additional Data

* ***Serverless-Webpack Version you're using***: 3.1.2
* ***Webpack version you're using***: 3.8.1
* ***Serverless Framework Version you're using***: 1.24
* ***Operating System***: Linux/Mac
* ***Stack Trace***:
```
Serverless: GET /v1/restaurants/1/onboarding (λ: getOnboarding)

Serverless: Error while loading getOnboarding

[ 'Error: Serverless-offline: handler for \'getOnboarding\' is not a function',
  'at Object.createHandler (/home/travis/build/deliveryhero/rps-kyc/node_modules/serverless-offline/src/functionHelper.js:38:13)',
  'at handler (/home/travis/build/deliveryhero/rps-kyc/node_modules/serverless-offline/src/index.js:499:40)',
  'at Object.internals.handler (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/handler.js:96:36)',
  'at request._protect.run (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/handler.js:30:23)',
  'at internals.Protect.run (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/protect.js:64:5)',
  'at exports.execute (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/handler.js:24:22)',
  'at each (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/request.js:384:16)',
  'at iterate (/home/travis/build/deliveryhero/rps-kyc/node_modules/items/lib/index.js:36:13)',
  'at done (/home/travis/build/deliveryhero/rps-kyc/node_modules/items/lib/index.js:28:25)',
  'at internals.Auth._authenticate (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/auth.js:210:16)',
  'at internals.Auth.authenticate (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/auth.js:202:17)',
  'at each (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/request.js:384:16)',
  'at iterate (/home/travis/build/deliveryhero/rps-kyc/node_modules/items/lib/index.js:36:13)',
  'at done (/home/travis/build/deliveryhero/rps-kyc/node_modules/items/lib/index.js:28:25)',
  'at internals.state (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/route.js:357:16)',
  'at each (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/request.js:384:16)',
  'at iterate (/home/travis/build/deliveryhero/rps-kyc/node_modules/items/lib/index.js:36:13)',
  'at Object.exports.serial (/home/travis/build/deliveryhero/rps-kyc/node_modules/items/lib/index.js:39:9)',
  'at internals.Request._lifecycle (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/request.js:387:11)',
  'at internals.Request._execute (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/request.js:302:21)',
  'at Domain.request._protect.enter (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/connection.js:261:25)',
  'at Domain.run (domain.js:221:14)',
  'at internals.Protect.enter (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/protect.js:80:17)',
  'at Server.<anonymous> (/home/travis/build/deliveryhero/rps-kyc/node_modules/hapi/lib/connection.js:259:30)',
  'at emitTwo (events.js:106:13)',
  'at Server.emit (events.js:191:7)',
  'at HTTPParser.parserOnIncoming [as onIncoming] (_http_server.js:546:12)' ]
Serverless: Replying error in handler

```

Pull request:
Fixed uncontrolled second compile when entering watch mode for offline.
<!--
1. Do not remove any section of the template. If something is not applicable leave it empty but leave it in the PR
2. Please follow the template, otherwise we'll have to ask you to update it and it will take longer until your PR is merged
-->

## What did you implement:

Closes #279 

<!--
Briefly describe the feature if no issue exists for this PR. If possible only
submit PRs for existing issues. If the PR is trivial (like doc changes or simple
code fixes) it can be submitted without a related issue, but as soon as it adds
or changes functionality, a related issue should be present.
-->

## How did you implement it:

<!--
If this is a nontrivial change please briefly describe your implementation so its easy for us to understand and review your code.
-->

`webpack.compiler.watch()` automatically triggers a compile. This led to a double compile (with the second one being uncontrolled and asynchronous) on startup of the plugin together with serverless offline. The second unwanted compile was running in parallel with the serverless-offline start.

Now, for watch mode, the inital compile is skipped as the watch will do it anyway. For non-watch mode the regular `webpack:compile` lifecycle is triggered.

Additionally a new switch `--webpack-no-watch` has been added to turn off the default watch mode with serverless-offline. This is needed, if you use serverless-offline in an automated build that does integration tests (and changes the source tree) with an external script given to offline. There you only want to build once and test the output with `serverless offline --exec` automatically. As soon as the given script changes the source tree, it leads to a recompile which breaks the script.

## How can we verify it:

<!--
Add any applicable config, commands, screenshots or other resources
to make it easy for us to verify this works. The easier you make it for us
to review a PR, the faster we can review and merge it.

Examples:
* Step by step description, how to verify
* Screenshots - Showing the difference between your output and the master
* Other - Anything else that comes to mind to help us evaluate
-->

Use `serverless offline` or `serverless offline start` and check that there is only one compile in the beginning (and not two) and that the plugin waits for its completion. Only then the offline plugin should start its work (watch mode enabled as before).

Use `serverless offline --webpack-no-watch` to start with watch mode disabled. Changes in the sources should not trigger a rebuild now.

## Todos:

- [x] Write tests
- [x] Write documentation
- [x] Fix linting errors
- [x] Make sure code coverage hasn't dropped
- [x] Provide verification config / commands / resources
- [x] Enable "Allow edits from maintainers" for this PR
- [x] Update the messages below

***Is this ready for review?:*** YES
***Is it a breaking change?:*** NO


Patch:
diff --git a/README.md b/README.md
index fcd34fe..5dc3aea 100644
--- a/README.md
+++ b/README.md
@@ -361,6 +361,10 @@ In comparison to `serverless offline`, the `start` command will fire an `init` a
 
 You can find an example setup in the [`examples`][link-examples] folder.
 
+By default the plugin starts in watch mode when triggered through `serverless offline`, i.e.
+it automatically recompiles your code if it detects a change in the used sources.
+After a change it might take some seconds until the emulated endpoints are updated.
+
 If you have your sources located on a file system that does not offer events,
 e.g. a mounted volume in a Docker container, you can enable polling with the
 `--webpack-use-polling=<time in ms>` option. If you omit the value, it defaults
@@ -389,6 +393,9 @@ Run `serverless offline start`.
 You can reduce the clutter generated by `serverless-offline` with `--dontPrintOutput` and
 disable timeouts with `--noTimeout`.
 
+If you use serverless offline to run your integration tests, you might want to
+disable the automatic watch mode with the `--webpack-no-watch` switch.
+
 ### Bundle with webpack
 
 To just bundle and see the output result use:
diff --git a/index.js b/index.js
index 5531c09..8733290 100644
--- a/index.js
+++ b/index.js
@@ -129,12 +129,10 @@ class ServerlessWebpack {
 
       'before:offline:start': () => BbPromise.bind(this)
         .then(this.prepareOfflineInvoke)
-        .then(() => this.serverless.pluginManager.spawn('webpack:compile'))
         .then(this.wpwatch),
 
       'before:offline:start:init': () => BbPromise.bind(this)
         .then(this.prepareOfflineInvoke)
-        .then(() => this.serverless.pluginManager.spawn('webpack:compile'))
         .then(this.wpwatch),
 
     };
diff --git a/lib/wpwatch.js b/lib/wpwatch.js
index 2428cd3..2325cfa 100644
--- a/lib/wpwatch.js
+++ b/lib/wpwatch.js
@@ -6,7 +6,13 @@ const webpack = require('webpack');
 
 module.exports = {
   wpwatch() {
-    this.serverless.cli.log('Watching with Webpack...');
+    if (this.options['webpack-no-watch']) {
+      // If we do not watch we will just run an ordinary compile
+      this.serverless.cli.log('Watch disabled by option.');
+      return this.serverless.pluginManager.spawn('webpack:compile');
+    }
+
+    this.serverless.cli.log('Bundling with Webpack...');
 
     const watchOptions = {};
     const usePolling = this.options['webpack-use-polling'];
@@ -16,16 +22,41 @@ module.exports = {
     }
 
     const compiler = webpack(this.webpackConfig);
-    compiler.watch(watchOptions, (err, stats) => {
-      if (err) {
-        throw err;
-      }
-
-      if (stats) {
-        this.serverless.cli.consoleLog(stats.toString(this.webpackConfig.stats));
-      }
-    });
+    const consoleStats = this.webpackConfig.stats || {
+      colors: true,
+      hash: false,
+      version: false,
+      chunks: false,
+      children: false
+    };
+
+    // This starts the watch and waits for the immediate compile that follows to end or fail.
+    const startWatch = (callback) => {
+      let firstRun = true;
+      compiler.watch(watchOptions, (err, stats) => {
+        if (err) {
+          if (firstRun) {
+            firstRun = false;
+            return callback(err);
+          }
+          throw err;
+        }
 
-    return BbPromise.resolve();
+        if (stats) {
+          this.serverless.cli.consoleLog(stats.toString(consoleStats));
+        }
+
+        this.serverless.cli.log('Watching for changes...');
+
+        if (firstRun) {
+          firstRun = false;
+          callback();
+        }
+      });
+    };
+
+    return BbPromise.fromCallback(cb => {
+      startWatch(cb);
+    });
   },
 };


Test patch:
diff --git a/tests/all.js b/tests/all.js
index 649a3ff..b816bd6 100644
--- a/tests/all.js
+++ b/tests/all.js
@@ -7,4 +7,5 @@ describe('serverless-webpack', () => {
   require('./packExternalModules.test');
   require('./run.test');
   require('./cleanup.test');
+  require('./wpwatch.test');
 });
diff --git a/tests/fs-extra.mock.js b/tests/fs-extra.mock.js
deleted file mode 100644
index e640aad..0000000
--- a/tests/fs-extra.mock.js
+++ /dev/null
@@ -1,14 +0,0 @@
-'use strict';
-
-const sinon = require('sinon');
-
-module.exports = () => ({
-  _resetSpies() {
-    for (let p in this) {
-      if (this.hasOwnProperty(p) && p !== '_resetSpies') {
-        this[p].reset();
-      }
-    }
-  },
-  removeSync: sinon.spy(),
-});
diff --git a/tests/mocks/fs-extra.mock.js b/tests/mocks/fs-extra.mock.js
index fece835..7d012c0 100644
--- a/tests/mocks/fs-extra.mock.js
+++ b/tests/mocks/fs-extra.mock.js
@@ -7,7 +7,8 @@
 module.exports.create = sandbox => {
   const fsExtraMock = {
     copy: sandbox.stub().yields(),
-    pathExists: sandbox.stub().yields()
+    pathExists: sandbox.stub().yields(),
+    removeSync: sandbox.stub()
   };
 
   return fsExtraMock;
diff --git a/tests/validate.test.js b/tests/validate.test.js
index ad5be2b..de2ea28 100644
--- a/tests/validate.test.js
+++ b/tests/validate.test.js
@@ -6,7 +6,7 @@ const sinon = require('sinon');
 const mockery = require('mockery');
 const path = require('path');
 const Serverless = require('serverless');
-const makeFsExtraMock = require('./fs-extra.mock');
+const fsExtraMockFactory = require('./mocks/fs-extra.mock');
 
 chai.use(require('sinon-chai'));
 
@@ -27,7 +27,7 @@ describe('validate', () => {
     sandbox = sinon.sandbox.create();
 
     mockery.enable({ warnOnUnregistered: false });
-    fsExtraMock = makeFsExtraMock();
+    fsExtraMock = fsExtraMockFactory.create(sandbox);
     mockery.registerMock('fs-extra', fsExtraMock);
     mockery.registerMock('glob', globMock);
     baseModule = require('../lib/validate');
@@ -44,7 +44,6 @@ describe('validate', () => {
     serverless.cli = {
       log: sandbox.stub()
     };
-    fsExtraMock._resetSpies();
     module = _.assign({
       serverless,
       options: {},
diff --git a/tests/webpack.mock.js b/tests/webpack.mock.js
index f535748..1939f38 100644
--- a/tests/webpack.mock.js
+++ b/tests/webpack.mock.js
@@ -1,13 +1,15 @@
 'use strict';
 
-const StatsMock = sandbox => ({
+const sinon = require('sinon');
+
+const StatsMock = () => ({
   compilation: {
     errors: [],
     compiler: {
       outputPath: 'statsMock-outputPath',
     },
   },
-  toString: sandbox.stub().returns('testStats'),
+  toString: sinon.stub().returns('testStats'),
 });
 
 const CompilerMock = (sandbox, statsMock) => ({
@@ -18,7 +20,7 @@ const CompilerMock = (sandbox, statsMock) => ({
 const webpackMock = sandbox => {
   const statsMock = StatsMock(sandbox);
   const compilerMock = CompilerMock(sandbox, statsMock);
-  const mock = sandbox.stub().returns(compilerMock);
+  const mock = sinon.stub().returns(compilerMock);
   mock.compilerMock = compilerMock;
   mock.statsMock = statsMock;
   return mock;
diff --git a/tests/wpwatch.test.js b/tests/wpwatch.test.js
new file mode 100644
index 0000000..4645c04
--- /dev/null
+++ b/tests/wpwatch.test.js
@@ -0,0 +1,195 @@
+'use strict';
+
+const BbPromise = require('bluebird');
+const _ = require('lodash');
+const chai = require('chai');
+const sinon = require('sinon');
+const mockery = require('mockery');
+const Serverless = require('serverless');
+const makeWebpackMock = require('./webpack.mock');
+const makeUtilsMock = require('./utils.mock');
+
+chai.use(require('chai-as-promised'));
+chai.use(require('sinon-chai'));
+
+const expect = chai.expect;
+
+describe('wpwatch', function() {
+  let sandbox;
+  let webpackMock;
+  let utilsMock;
+  let baseModule;
+  let serverless;
+  let module;
+  let spawnStub;
+
+  this.timeout(7000);
+
+  before(() => {
+    sandbox = sinon.createSandbox();
+    sandbox.usingPromise(BbPromise.Promise);
+
+    webpackMock = makeWebpackMock(sandbox);
+    utilsMock = makeUtilsMock();
+
+    mockery.enable({ warnOnUnregistered: false });
+    mockery.registerMock('webpack', webpackMock);
+    mockery.registerMock('./utils', utilsMock);
+    baseModule = require('../lib/wpwatch');
+    Object.freeze(baseModule);
+  });
+
+  after(() => {
+    mockery.disable();
+    mockery.deregisterAll();
+  });
+
+  beforeEach(() => {
+    serverless = new Serverless();
+    serverless.cli = {
+      log: sandbox.stub(),
+      consoleLog: sandbox.stub()
+    };
+
+    module = _.assign({
+      serverless,
+      options: {},
+    }, baseModule);
+
+    spawnStub = sandbox.stub(serverless.pluginManager, 'spawn');
+
+    const webpackConfig = {
+      stats: 'minimal'
+    };
+    _.set(module, 'webpackConfig', webpackConfig);
+  });
+
+  afterEach(() => {
+    // This will reset the mocks too
+    webpackMock.compilerMock.watch.reset();
+    sandbox.restore();
+  });
+
+  it('should reject if webpack watch fails', () => {
+    const wpwatch = module.wpwatch.bind(module);
+    webpackMock.compilerMock.watch.yields(new Error('Failed'));
+
+    return expect(wpwatch()).to.be.rejectedWith('Failed');
+  });
+
+  it('should spawn compile if watch is disabled', () => {
+    const wpwatch = module.wpwatch.bind(module);
+    webpackMock.compilerMock.watch.yields(null, {});
+    spawnStub.resolves();
+    _.set(module.options, 'webpack-no-watch', true);
+
+    return expect(wpwatch()).to.be.fulfilled
+    .then(() => BbPromise.join(
+      expect(spawnStub).to.have.been.calledWith('webpack:compile'),
+      expect(webpackMock.compilerMock.watch).to.not.have.been.called
+    ));
+  });
+
+  it('should enter watch mode and return after first compile', () => {
+    const wpwatch = module.wpwatch.bind(module);
+    webpackMock.compilerMock.watch.yields(null, {});
+    spawnStub.resolves();
+
+    return expect(wpwatch()).to.be.fulfilled
+    .then(() => BbPromise.join(
+      expect(spawnStub).to.not.have.been.called,
+      expect(webpackMock.compilerMock.watch).to.have.been.calledOnce
+    ));
+  });
+
+  it('should work if no stats are returned', () => {
+    const wpwatch = module.wpwatch.bind(module);
+    webpackMock.compilerMock.watch.yields();
+    spawnStub.resolves();
+
+    return expect(wpwatch()).to.be.fulfilled
+    .then(() => BbPromise.join(
+      expect(spawnStub).to.not.have.been.called,
+      expect(webpackMock.compilerMock.watch).to.have.been.calledOnce
+    ));
+  });
+
+  it('should enable polling with command line switch', () => {
+    const wpwatch = module.wpwatch.bind(module);
+    webpackMock.compilerMock.watch.yields();
+    spawnStub.resolves();
+    _.set(module.options, 'webpack-use-polling', true);
+
+    return expect(wpwatch()).to.be.fulfilled
+    .then(() => BbPromise.join(
+      expect(spawnStub).to.not.have.been.called,
+      expect(webpackMock.compilerMock.watch).to.have.been.calledOnce,
+      expect(webpackMock.compilerMock.watch).to.have.been.calledWith({ poll: 3000 })
+    ));
+  });
+
+  it('should set specific polling interval if given with switch', () => {
+    const wpwatch = module.wpwatch.bind(module);
+    webpackMock.compilerMock.watch.yields();
+    spawnStub.resolves();
+    _.set(module.options, 'webpack-use-polling', 5000);
+
+    return expect(wpwatch()).to.be.fulfilled
+    .then(() => BbPromise.join(
+      expect(spawnStub).to.not.have.been.called,
+      expect(webpackMock.compilerMock.watch).to.have.been.calledOnce,
+      expect(webpackMock.compilerMock.watch).to.have.been.calledWith({ poll: 5000 })
+    ));
+  });
+
+  it('should call callback on subsequent runs', () => {
+    const wpwatch = module.wpwatch.bind(module);
+    let watchCallbackSpy;
+    webpackMock.compilerMock.watch.callsFake((options, cb) => {
+      // We'll spy the callback registered for watch
+      watchCallbackSpy = sandbox.spy(cb);
+
+      // Schedule second call after 2 seconds
+      setTimeout(() => {
+        watchCallbackSpy(null, { call: 2 });
+      }, 2000);
+      process.nextTick(() => watchCallbackSpy(null, { call: 1 }));
+    });
+    spawnStub.resolves();
+
+    return expect(wpwatch()).to.be.fulfilled
+    .then(() => BbPromise.delay(3000))
+    .then(() => BbPromise.join(
+      expect(spawnStub).to.not.have.been.called,
+      expect(webpackMock.compilerMock.watch).to.have.been.calledOnce,
+      expect(watchCallbackSpy).to.have.been.calledTwice
+    ));
+  });
+
+  it('should throw if compile fails on subsequent runs', () => {
+    const wpwatch = module.wpwatch.bind(module);
+    let watchCallbackSpy;
+    webpackMock.compilerMock.watch.callsFake((options, cb) => {
+      // We'll spy the callback registered for watch
+      watchCallbackSpy = sandbox.spy(cb);
+
+      // Schedule second call after 2 seconds
+      setTimeout(() => {
+        try {
+          watchCallbackSpy(new Error('Compile failed'));
+        } catch (e) {
+          // Ignore the exception. The spy will record it.
+        }
+      }, 2000);
+      process.nextTick(() => watchCallbackSpy(null, { call: 1 }));
+    });
+    spawnStub.resolves();
+
+    return expect(wpwatch()).to.be.fulfilled
+    .then(() => BbPromise.delay(3000))
+    .then(() => BbPromise.join(
+      expect(watchCallbackSpy).to.have.been.calledTwice,
+      expect(watchCallbackSpy.secondCall.threw()).to.be.true
+    ));
+  });
+});