Feature Proposal: Custom Functions Linter

[dkundel]

Background

Functions have a couple of oddities that can cause frustation for users. It would be useful if we can ship the Serverless Toolkit with a linter that helps customers avoid these during development.

Proposal

We will create a custom ESLint plugin that specifically focuses on Twilio Functions rules. It does not care about formatting but primarily focuses on catching things that would cause unintended behavior when deployed and executed.

Possible Rules

Error Rule: Requiring local files

In local development require('./someOtherFunction.js) will work but it will break once deployed. This rule should fire for any local require statements that don't use Runtime.getAssets() or Runtime.getFunctions() to retrieve the path. For example this would be the valid equivalent: require(Runtime.getFunctions()['someOtherFunction'].path)

Warn Rule: No return statement on callback

In theory when you call callback(...) the execution of the Function stops but in theory based on how the Node.js event loop works there could still be some work executed inside the Function. A return callback(...) at least significantly reduces the likelyhood.

This rule should already exist and just be included in this plugin

Warn Rule: process.env vs context.

While environment variables can be accessed both through process.env and through the context object, context is technically the recommended way. This rule would warn people about the yse of process.env to limit the impact of potential future behavior changes.

Error Rule: Referenced but missing dependency

Only dependencies listed in the dependencies field of the package.json will be deployed therefore we should throw an error when a package is required through require but isn't actually listed in the package.json.

Error Rule: No callback call in handler Function

If the Function handler declares a third callback argument it has to be called. This rule will check that there is at least one callback call in the function. Ideally it checks if it's reachable.

Warn Rule: Incompatible Twilio version

This one would probably require us to use TypeScript ESLint to catch but if a customer uses the client from context.getTwilioClient() to access an API that isn't part of the respective version of twilio that is installed in the project we should warn the customer before deploying.

Error Rule: Invalid Path for Assets or Functions

If you use Runtime.getAssets()[somePath] or Runtime.getFunctions()[somePath] you might use invalid path names because you forget to remove .private. from an Asset, format the Functions path wrong by including the / or even reference a non-existent Function/Asset in the first place. This rule should error out in those scenarios.

Other Rules

What are other rules we should include? We can add more going forward but we should check with others on what common gotchas are.

Usage

This should be packaged as a regular ESLint plugin so it can be used outside the Serverless Toolkit as well.
In new projects we should set them up with a pre deploy script that runs the linter.
For the Twilio CLI Plugin we could check if the project has a lint:functions script and automatically call it before deployment unless a --no-lint flag is called. This should be a backwards compatible change that way as existing projects would likely not have a lint:functions script.

[ktalebian]

One other rule (which might be hard to implement), but a lot of customers mis-use the callback and promises and so things like

// this is a promise - like an API call
doSomething();
callback();

And so this stops the promise invocation because the callback is called. If we could catch it, that would be amazing.

[ShelbyZ]

One other rule (which might be hard to implement), but a lot of customers mis-use the callback and promises and so things like

// this is a promise - like an API call
doSomething();
callback();

And so this stops the promise invocation because the callback is called. If we could catch it, that would be amazing.

The best I can think of is catching unawaited promises via require-await eslint rule and have the parent function be marked async. For cases where the intention is promise chaining we would need to look at eslint-plugin-no-floating-promise, but that may make detecting that the last executable statement is callback(), return callback(), or return problematic.