Skip to content
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

Feature/support nodejs18 esmodule #535

Closed
wants to merge 50 commits into from

Conversation

kai-nguyen-aligent
Copy link
Contributor

This PR support the following things:

  • Update Serverless framework to latest version (3.25.0) to support nodejs18.x runtime
  • Remove obsoleted source-map-support package as we can enable it by --enable-source-maps node runtime option.
  • Update dependencies to latest stable version, which include axios, aws-sdk v3, jest, ts-jest, etc…
  • Support ES Module format which enable top-level await.

While working on this, there are few caveats that worth to mentioned:

  1. While ES module format enable top-level await and significantly reduce the init billing duration, we have to use a hack to make it works otherwise we will not be able to use any native runtime library. This is an issue of esbuild as mention in this PR: Rewrite require() calls of Node built-ins to import statements when emitting ESM for Node by eduardoboucas · Pull Request #2067 · evanw/esbuild. For the time being, we need to tell esbuild to inject a banner like so to the output:
    import { createRequire } from 'module'; const require = createRequire(import.meta.url);

  2. If we have top-level await in our code, Jest mock will not work normally unless we do the following things:
    a. Tell node to run jest in --experimental-vm-modules mode.
    b. We have to use unstable_mockModule function to mock and then dynamically import our test modules as demonstrated in collect-ssm-params-tests.test.ts example.

  3. If we do not want to take advantage of top-level await (requires ES module format) or leverage the native aws-sdk v3 in AWS Lambda NodeJS18 runtime (reduce package size) then there is no significant reason to upgrade at the moment.

Copy link
Collaborator

@tvhees tvhees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm keen to move to node18 and ok with the esbuild hack required for top-level await, but this is not a clean PR.

@@ -1,3 +1,21 @@
#!/bin/sh

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheOrangePuff could you review this section?

@@ -0,0 +1,46 @@
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kai-nguyen-aligent there's a fair bit in this PR that doesn't seem related to the PR topic/description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tvhees So sorry. I think it's my bad when I create this branch. Look like it include the changes from the other PR. Let me create another one with only the changes for node18 & esm

@@ -1,15 +1,30 @@
import 'source-map-support/register';
import axios from 'axios';
import { getConfigurations } from './lib/collect-ssm-params';

// AWSLambda.Handler is provides very generic typing
// for handler functions. Specific argument and output
// types can be supplied using generic arguments
// e.g. AWSLambda.Handler<string, object>, and/or using
// event-specific handler types e.g. AWSLambda.S3Handler
export const handler: AWSLambda.Handler = async (event, context) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that GitHub is throwing errors for this - that's typescript complaining I assume? We should probably check that in the main template.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got this complaining by both typescript & eslint.

'context' is defined but never used.eslint[@typescript-eslint/no-unused-vars](https://typescript-eslint.io/rules/no-unused-vars) 'context' is declared but its value is never read.ts(6133)

@kai-nguyen-aligent
Copy link
Contributor Author

My bad while creating this branch. It included some of my local modification & seem to be branch off the debug-support branch. Let me close this one and create another one from main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants