Feature/verbose handler loader

[emmoistner]

Verbose Handler Loader

Backstory

Node v4.0 has some trouble with displaying lineNumbers, fileNames from code that was dynamically required, like this project does. More information on the issues (nodejs/node#2762).

Goal

Provide devs an option to receive better stack traces from SyntaxErrors.

implementation

A new parameter, --verboseHandlerLoader (Default: false).
When provided, it will run a different loader function. If the code fails to load, it will spawn a childProcess node ${handlerPath}.js attempting to retrieve more in depth debug/stack information.

By default the previous handlerLoader is ran which only requires the handlerModule.

Additional bugfixes (tests)

Added before and after hooks to integration tests to handle restoring functionHelper.createHandler

Commits are broken up so this can be easily separated out.

Notes

• I used a server option, because I was attempting to keep the default api unchanged and staying with with the theme. Emulation of the lambda environment is the key.

Why child process execSync node?

• I attempted use a couple different modules, one such as syntax-error, but they required building a dependency tree. Running node filename.js seemed to be the better direction since it still gives decent stack traces if modules are eager loaded.

Screenshots

Example Manual Test Setup (Replication)

For example replace line 46 of manual_test/handler.js with an intentional SyntaxError like

module.exports.catchAll = (event, context, cb) => {
  const response = {
    statusCode: 200,
    body: JSON.stringify({
      message: 'Catch all route'
      stuff: 'hello'
    }),
  };

  cb(null, response);
};

Before

After (default: without --verboseHandlerLoader)

After (with --verboseHandlerLoader)

Any suggestions would be greatly appreciated.

[dherault]

Hi @emmoistner thanks for the PR.
The feature is great, I have a few questions before merging:

• Why make it an activable feature, why not making it the new behavior ? Why would anyone choose not to use that ?
• What are the caveats ? Is it slower ?
• Do you like intense questioning ? ;)

Thanks

[emmoistner]

Great questions.

Brief

1. Assumptions
   a. file extensions for handlerPaths are always .js files?
   b. What would the lambda output look like if given a handler with a syntaxError (or required dependency with a SyntaxError)?
2. Caveats
   a. Execution Time - Possibly slower on failures, same time as today with a successful load
   b. Removal - This code should be removed if ever using node6+

I would be good with making this the default loader if 1.a is true, 1.b matches output from the verboseHandlerLoader or we are okay with stack traces being better locally, and we are okay with the caveats.

In Depth

1. Two assumptions why I landed at an activatable feature.
   file extensions If the handlerPath is passed without an extension, I naively insert .js and assume it's valid. I'm not fully intimate with serverless and its options for transpiling & bundling. I was hoping you might have some feedback on it.
   emulation of lambda environment Since Offline is attempting to emulate the lambda environment. I wanted to keep this a similar as possible even if bad stack traces are the reality.
   This does leave me with question that should be answered. How would lambda actually handle a syntaxerror and what would the stack trace look like?

2. Caveats
   Execution Time
   Success - Same as before
   Failure - Could potentially double Higher execution time, but this is depends on how much handler code needs to be loaded and where the error is located in code.
   Update -
   Examples with Manual Test:
   verboseHandlerLoader: off - Execution time - 0ms - 15ms
   verboseHandlerLoader: On - Execution time - 50ms - 60ms
   This execution time could probably decreased by using async code
   Removal - This code is designed specifically for node 4.x - 5.x. If aws ever updates there runtime to node 6+, this code can be removed.

3. Ya no problem!

[emmoistner]

@dherault Ping, any thoughts?

[dherault]

@emmoistner sorry for the wait. I still think this is too much diff for a single feature. Moreover, I'm working on v4 to tackle te code quality problem. The idea is great thought.

[emmoistner]

@dherault Just a final note. If you decide to go with the same workflow, you will most likely run into the same problems with base syntax errors with empty stack traces. It just something to think about when using node 4.x

Reference: nodejs/node#2762