Open dkundel opened 3 years ago
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.
One other rule (which might be hard to implement), but a lot of customers mis-use the
callback
andpromises
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.
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 useRuntime.getAssets()
orRuntime.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. Areturn 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 ofprocess.env
to limit the impact of potential future behavior changes.Error Rule: Referenced but missing dependency
Only dependencies listed in the
dependencies
field of thepackage.json
will be deployed therefore we should throw an error when a package is required throughrequire
but isn't actually listed in thepackage.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 onecallback
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 oftwilio
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]
orRuntime.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 alint:functions
script.