feat(nextjs-cdk-construct): add runtime options to regeneration #2050
Conversation
…unction so that it can be modified from options props
| role: this.edgeLambdaRole, | ||
| runtime: | ||
| toLambdaOption("regenerationLambda", props.runtime) ?? | ||
| lambda.Runtime.NODEJS_12_X, |
There was a problem hiding this comment.
I'm not 100% sure why it was hardcoded as NODEJS_14_X but I think it's unintuitive that the regeneration function is the only one with a fixed nodejs version. This might result in some weird bugs if it is the regeneration function uses a different runtime than the default function because they use the same js page bundles. This line I added is the same as for the other functions and it makes sure that all functions use the same nodejs version except if the user really wants to set something different throught props
There was a problem hiding this comment.
Don't we want the regeneration lamda have the same role as the other functions? I know in my case I need this because the role grants access to dynamo to get the date necessary for generating a page
There was a problem hiding this comment.
Sorry @mathisobadia to be clear I think what @dmsolutionz is saying is that the "default should remain as NODEJS_14_X" rather than it shouldn't be editable. If you change to NODEJS_12_X as the default that's a breaking change.
Having it as editable like the others is exactly the desired outcome here.
There was a problem hiding this comment.
Oh yeah that makes perfect sense! I've updated my pull request accordingly.
Thank you for the feedback! I can change my pull request but I'm wondering why this function should be the only one with fixed node 14 runtime while all the other ones can be set with props? it doesn't make sense to keep the props as they are with the possibility to set the runtime version of the regeneration function since we now have /**
* Lambda runtimes(s)
*/
runtime?: LambdaOption<Runtime>;and export type LambdaOption<T> =
| T
| {
defaultLambda?: T;
apiLambda?: T;
imageLambda?: T;
regenerationLambda?: T;
};this means that the user can use a prop like runtime: {
regenerationLamda: Runtime.NODEJS_12_X
}this would be valid typescript prop but it would not change the runtime of the function. This would be highly deceptive. As an alternative, so that we don't change the runtime of current users unexpectedly we could keep the option to set the runtime but keep as a default the NODEJS_14_X instead of the default NODEJS_12_X that we have for all the other lambdas |
|
I just noticed that the tests fail because of the snapshots I've updated the pull request to put back the previous default memory (undefined) and remove the unnecessary role that I added by mistake. This should now pass the tests |
Handler Size Report
Base Handler Sizes (kB) (commit f63406e){
"Lambda": {
"Default Lambda": {
"Standard": 1523,
"Minified": 668
},
"Image Lambda": {
"Standard": 1487,
"Minified": 801
}
},
"Lambda@Edge": {
"Default Lambda": {
"Standard": 1533,
"Minified": 673
},
"Default Lambda V2": {
"Standard": 1525,
"Minified": 670
},
"API Lambda": {
"Standard": 634,
"Minified": 318
},
"Image Lambda": {
"Standard": 1495,
"Minified": 806
},
"Regeneration Lambda": {
"Standard": 1183,
"Minified": 544
},
"Regeneration Lambda V2": {
"Standard": 1253,
"Minified": 572
}
}
}New Handler Sizes (kB) (commit c4bc4cf){
"Lambda": {
"Default Lambda": {
"Standard": 1523,
"Minified": 668
},
"Image Lambda": {
"Standard": 1487,
"Minified": 801
}
},
"Lambda@Edge": {
"Default Lambda": {
"Standard": 1533,
"Minified": 673
},
"Default Lambda V2": {
"Standard": 1525,
"Minified": 670
},
"API Lambda": {
"Standard": 634,
"Minified": 318
},
"Image Lambda": {
"Standard": 1495,
"Minified": 806
},
"Regeneration Lambda": {
"Standard": 1183,
"Minified": 544
},
"Regeneration Lambda V2": {
"Standard": 1253,
"Minified": 572
}
}
} |
Codecov Report
@@ Coverage Diff @@
## master #2050 +/- ##
==========================================
+ Coverage 83.51% 83.53% +0.02%
==========================================
Files 102 102
Lines 3658 3663 +5
Branches 1154 1159 +5
==========================================
+ Hits 3055 3060 +5
Misses 592 592
Partials 11 11
Continue to review full report at Codecov.
|
fixes issue #2048
The regeneration function was the only function where the runtime could not be modified through setttings prop
This can cause the regeneration lambda function to time out an fail
The pull request adds the option to configure the regeneration runtime (memory, timeout, role, and node version)
as a note I have not modified the nomenclature for this function but it is the only lambda handler that is named as Function instead of Lambda for the others (nextApiLambda, nextDefaultLambda, nextImageLambda) but the regeneration lambda is named RegenerationFunction. I can modify that also if needed.