feat(cloudfront): write Lambda@Edge with typescript #18836
Conversation
|
|
|
Will start writing the tests if that PR hits interested and after we are happy with the production code part. |
|
@comcalvi can I have some feedback for how to get rid of those duplicates? |
mmuller88
changed the title
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
rix0rrr
added
feature-request
| @@ -0,0 +1,208 @@ | |||
| import * as fs from 'fs'; | |||
| import * as path from 'path'; | |||
| import * as cloudfront from '@aws-cdk/aws-cloudfront'; | |||
There was a problem hiding this comment.
I think this import: import * as cloudfront from '@aws-cdk/aws-cloudfront'; is causing the build to fail. This file is in the aws-cloudfront module, so you shouldn't import from @aws-cdk/aws-cloudfront. Instead you should import the classes you need relative to the current directory.
| code: bundling.Bundling.bundle({ | ||
| ...props.bundling ?? {}, | ||
| architecture, | ||
| runtime, | ||
| depsLockFilePath, | ||
| entry, | ||
| projectRoot, | ||
| }), | ||
| handler: `index.${handler}`, |
There was a problem hiding this comment.
This is weird...why are we finding all of these values ourselves? The code property is a lambda function code property, which should be created using lambda's existing methods (eg lambda.Code.fromAsset(), or something else). In general, I think we should let the user specify their code directly instead of trying to find it for them.
| * 3. A .js file name as the defining file with id as suffix (defining-file.id.js) | ||
| * 4. A .mjs file name as the defining file with id as suffix (defining-file.id.mjs) | ||
| */ | ||
| function findEntry(id: string, entry?: string): string { |
There was a problem hiding this comment.
Again, I don't think we should be finding any of these files ourselves. The user should be specifying these directly.
There was a problem hiding this comment.
That's what NodejsFunction does, but the findEntry helper is not exposed for reuse. It makes sense to keep the same behavior for both NodejsFunction and NodejsEdgeFunction.
| * @default - Derived from the name of the defining file and the construct's id. | ||
| * If the `NodejsFunction` is defined in `stack.ts` with `my-handler` as id | ||
| * (`new NodejsFunction(this, 'my-handler')`), the construct will look at `stack.my-handler.ts` | ||
| * and `stack.my-handler.js`. | ||
| */ | ||
| readonly entry?: string; |
There was a problem hiding this comment.
why? This seems to be something the user must have in order for this construct to deploy, so having it be an optional property is confusing. They should specify it directly.
There was a problem hiding this comment.
The NodejsFunction does the same as well: it looks for a specific entry file based on the file where the NodejsFunction is created.
| * | ||
| * @default true | ||
| */ | ||
| readonly awsSdkConnectionReuse?: boolean; |
There was a problem hiding this comment.
why do we need this? It's not referenced anywhere in this PR
| } | ||
|
|
||
| return sites[definingIndex].getFileName(); | ||
| } |
There was a problem hiding this comment.
Could we get a blank line at EOF?
|
@comcalvi thx a lot for looking into that. But please read and try to understand the description as that will help to answer most of your questions! |
|
This PR has been in BUILD FAILING for 21 days, and looks abandoned. It will be closed in 10 days if no further commits are pushed to it. |
|
@mmuller88 before we get into the details of the code duplication, please update the PR so that the code build job succeeds. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
github-actions
bot
added
the
closed-for-staleness
|
@comcalvi I didn't feel well supported by you. You didn't understand or looked into much of what I was asking for or didn't understand it and clarified that. I kind of lost interest in this PR because of that. You are perhaps quite Junior so then I could understand that. A bit disappointed with you and AWS |
|
Similar issue here #14215 |
|
@comcalvi sorry for my stupid words. I think I was a bit frustrated and acted quite childish. I should have acted on your comments better and with more patience. I think I was frustrated that it took you so long for looking over my issue. But of course, the CDK team has lots to do. Again, I am very sorry. |
|
sorry all! I hadn't seen these since the PR was autoclosed. @mmuller88 @TheOrangePuff do you want me to re-open this PR? |
|
@comcalvi Yes please I think there's still no solution to this |
fixes: #12671
As you can see I duplicated a lot from aws-lambda-nodejs/lib/function.ts . It would be great to get rid of that. Let me know how to proceed.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license