-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(lambda-nodejs): command hooks #11583
Conversation
Add support for running additional commands before bundling, before node modules installation and after bundling. Closes aws#11468
* | ||
* Commands are chained with `&&`. | ||
*/ | ||
beforeBundling?(inputDir: string, outputDir: string): string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does these optional methods work with jsii?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It builds but in the .jsii
I don't see any optional: true
added to the method's type definition.
Also this pattern is nowhere to be seen in the codebase currently.
Does it mean that it will work for TS but other languages will have to define the 3 methods if commandHooks
is specified? Can we live with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RomainMuller thought?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional methods are not a thing in most languages, so we can't really support that. This is a TypeScript syntax I hadn't seen before... so I guess this is why the compiler does not complain...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eladb should we make the methods mandatory then or leave it like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make them mandatory so all languages will have a similar experience.
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
resolved the merge conflict |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
@ryanholte we are currently blocked from releasing new versions until re:invent is over. Sorry :-( |
Add support for running additional commands before bundling, before node
modules installation and after bundling.
Closes #11468
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license