feat: extract runtime-handler and lazyLoading (#252) #271
Conversation
dkundel
force-pushed
the
features/runtime-handler
branch
from
77eefc3 to
981b3b1
Compare
dkundel
force-pushed
the
features/runtime-handler
branch
from
981b3b1 to
f0ac4c0
Compare
- [email protected] - @twilio-labs/[email protected] - @twilio-labs/[email protected] - @twilio/[email protected] - @twilio-labs/[email protected] - @twilio-labs/[email protected] - [email protected]
There was a problem hiding this comment.
For the most part, this is a copy of twilio-run functionality to runtime-handler and that all seems fine to me. Bringing the lazy loading option in looks just fine to me.
We are duplicating some types, which I wonder if we could bring together somewhere.
And there are some unused dependencies in runtime-handler, which we could remove before publishing.
Otherwise, good lift here!
| @@ -0,0 +1,88 @@ | |||
| export type SearchConfig = { | |||
There was a problem hiding this comment.
runtime-handler is now exporting some duplicate types that also live in serverless-api or twilio-run. Should they be consolidated somewhere?
Also in serverless-api:
SearchConfig, AccessOptions, ServerlessResourceConfig, ServerlessResourceConfigWithFilePath
Also in twilio-run:
EnvironmentVariablesWithAuth
There was a problem hiding this comment.
The problem is the way the dependency chain works. twilio-run should rely on serverless-api and runtime-handler (at least during compile time) but runtime-handler shouldn't rely on twilio-run. And similarly I didn't want runtime-handlerto rely on serverless-api since runtime-handler can be different from user to user during runtime so it should be self-contained in general
There was a problem hiding this comment.
In fact you'll see that runtime-handler is only a devDependency in twilio-run because I do import type as opposed to import.
There was a problem hiding this comment.
Could runtime-handler take serverless-api as a devDependency and do import type too then?
There was a problem hiding this comment.
Unfortunately not because some of these are part of ServerConfig which is exported by runtime-handler. Realistically the only thing we could do is to move it into a common dependency that all of them consume. But realistically they are all part of ServerConfig which is basically the "contract" for runtime-handler. So I could see scenarios where they could diverge slightly from the other use cases.
I was able to remove SearchConfig though since it's not being used anymore.
| import { join } from 'path'; | ||
|
|
||
| export function requireFromProject(projectDir: string, packageName: string) { | ||
| return createRequire(join(projectDir, 'node_modules'))(packageName); |
There was a problem hiding this comment.
This will still fallback to the version installed by twilio-run if there isn't an explicit dependency on twilio in the project, right? And that is the intention?
There was a problem hiding this comment.
I made the fallback more explicit now.
There was a problem hiding this comment.
Have you pushed? The last commits I can see are from 13 days ago.
There was a problem hiding this comment.
Sorry I was changing around logic afterwards. It's now up-to-date
Replaced with a looser test based on the content at the start of the error message that should be thrown.
This change introduces the following features:
@twilio/runtime-handler@twilio/runtime-handlerto bring parity with the 1.1.0 release in Functionstwiliopackage instead of the one from the Serverless ToolkitContributing to Twilio