aws-cloudformation / cloudformation-coverage-roadmap

The AWS CloudFormation Public Coverage Roadmap
https://aws.amazon.com/cloudformation/
Creative Commons Attribution Share Alike 4.0 International
1.1k stars 53 forks source link

Add support for ECMAScript modules in inline ZipFile code #1420

Open lpsinger opened 1 year ago

lpsinger commented 1 year ago

Name of the resource

AWS::Lambda::Function

Resource name

No response

Description

The Node.js Lambda runtime, like Node.js itself, supports either CommonJS or ECMAScript module source code. The two are differentiated mainly by how packages are imported. The former uses the require function, where the latter uses the import statement. The latter is seen in some circles as more modern. Which module format is used is indicated by the file extension: index.js or index.cjs for CommonJS, or index.mjs for ECAMScript module.

The AWS::Lambda::Function CloudFormation resource supports embedded, inline source code via the Properties: Code: ZipFile. However, there is currently no way to tell CloudFormation that the embedded source code is an ECMAScript module because the file extension is always .js, not .mjs.

Add a property to control the file extension of the index file in the embedded source.

Other Details

Example using ECMAScript module syntax:

  PostConfirmSignupFunction:
    Type: AWS::Lambda::Function
    Properties:
      Code:
        ZipFile: |
          import {
            AdminAddUserToGroupCommand,
            CognitoIdentityProviderClient,
          } from '@aws-sdk/client-cognito-identity-provider'

          const client = new CognitoIdentityProviderClient({})
          const GroupName = 'defaultGroup'

          export async function handler(event) {
            const UserPoolId = event.userPoolId
            const Username = event.userName

            const command = new AdminAddUserToGroupCommand({
              UserPoolId, Username, GroupName
            })

            await client.send(command)
            return event
          }
      Handler: index.handler
      PackageType: Zip
      Role: !GetAtt PostConfirmSignupRole.Arn
      Runtime: nodejs18.x
jtuliani commented 1 year ago

@lpsinger Thanks for raising this. It's a good suggestion.

Question: do you prefer to be able to specify .mjs vs .cjs explicitly, or would it be better/acceptable for Lambda/CloudFormation to infer the correct suffix based on the presence of import/require statements in the inline code?

lpsinger commented 1 year ago

How about both! Proposed behavior:

everett1992 commented 1 year ago

I do not recommend auto inference. Checking of javascript content is ESM or CJS would require parsing to an AST and checkiing import / export statements. I don't think it can be safely done without an ECMAScript parser, and that would make CFN too language aware.

abiodunakande commented 1 year ago

Any update on this? Having the same issue - I would like to be able to define

HandlerFile: index.mjs
HandlerExport: handler
trparchman commented 1 year ago

Just now discovering the same thing mentioned above. I would be ever so much happier if I could explicitly indicate the use of ECMAScript module syntax when the code is an inline ZipFile.

luizcarlosrodrigues commented 12 months ago

Same problem here. looking for a fix

jtuliani commented 12 months ago

@lpsinger @luizcarlosrodrigues @trparchman @abiodunakande To help us prioritize, it would be useful if you can explain the impact of not supporting ES modules in inline functions in CloudFormation templates. Is it an inconvenience, or a blocker? Why is CommonJS not suitable as an alternative? Thank you for sharing.

abiodunakande commented 12 months ago

top level await uses es module syntax

luizcarlosrodrigues commented 12 months ago

For me is about changing all code that I already have to automate my deploy with SAM and cloudformation.

trparchman commented 11 months ago

For MANY years now the developer community has been moving toward ECMA/module Javascript. Most of the modern innovations are syntactically far clearer and less subject to error in the modern syntax. Some shops might not use or train their personnel to ever use Common JS in any of their development. To require such shops to use an unfamiliar syntax in order to use IaC with Lambda is a recipe for error, confusion, security breaches, and wasted time.

jtuliani commented 3 weeks ago

We are considering changing the default file name from index.js to index.mjs starting from the Node.js 22 runtime. Earlier runtimes up to Node.js 20 would continue to use index.js, to retain compatibility with existing CloudFormation templates.

We invite you to provide feedback on this approach. If this does not suffice for you, please provide as much detail as you can as to why.

trparchman commented 2 weeks ago

Changing the default is better late than never.

That said, adding a parameter to the CloudFormation syntax to allow the Common JS vs. ECMA/Module JS variant to be selected explicitly regardless of Node version would allow for the most flexibility, certainty, and environment compatibility.

jtuliani commented 1 week ago

Thank you @trparchman. Can you think of any situation where a customer creating an inline function could not use .mjs and would require CommonJS?

Adding a parameter has downsides--it adds complexity, we also need to consider how it affects the Python experience, and as the push towards ESM continues, it should eventually no longer be required.

tmokmss commented 1 week ago

It will require some manual work to migrate a function to ESM if a customer is using cfn-response module as described in this doc. I'm not strongly against defaulting ESM though.

"ZipFile": { "Fn::Join": ["", [
  "var response = require('cfn-response');",
  "exports.handler = function(event, context) {",
  "  var input = parseInt(event.ResourceProperties.Input);",
  "  var responseData = {Value: input * 5};",
  "  response.send(event, context, response.SUCCESS, responseData);",
  "};"
]]}
trparchman commented 1 week ago

Thank you @trparchman. Can you think of any situation where a customer creating an inline function could not use .mjs and would require CommonJS?

Adding a parameter has downsides--it adds complexity, we also need to consider how it affects the Python experience, and as the push towards ESM continues, it should eventually no longer be required.

No doubt that adding a parameter has downsides, especially as the long-term destination is MJS. The challenge is that that transition is not mandated by any authority, so there are plenty of shops that use the CJS syntax, notably with require directives for dependency management. Selfishly, I'd love to mandate the more expressive and consistent MJS approach, but there are probably tens of thousands of dev shops and millions of devs still using CJS who will have to suddenly read the memo and learn the MJS way if it becomes the only game in town for ZIPfile function bodies.

Essentially, the problem of MJS fans like myself who were smacked upside the head with the existing behavior when our code wouldn't work because it was placed in CJS/.js wrappers would be reversed, no? Seems like AWS doesn't want to be placed in the position of deciding who to confuse and anger because the Node/JavaScript community is not willing to deprecate and end-of-life CJS.

It's in that spirit that I suggest a parameter so you don't have to choose sides.