Closed flochaz closed 1 year ago
Thanks @flochaz for opening this issue! If I look at the AWS SDK v3, this is how @trivikr and the team handles it:
Full example for the DynamoDB client: https://github.com/aws/aws-sdk-js-v3/tree/cc17fa2296654192d2a9c2cf567d2ce58d81d15b/lib/lib-dynamodb
Personally, I think that right now supporting both CommonJS and ES6 is not a high priority, but a nice to have that would definitely improve the dev experience. I would like to see a small proof of concept but I don't think this is something we should prioritize for the production ready milestone. (Maybe some folks from our community can help out in the future?)
If we want, at least for now, make sure that the three core utilities available (Logger
, Metrics
, and Tracer
) all advance at the same pace, the main dependency for Tracer
(aws-xray-sdk-node
) constitutes a major blocker for this effort as it currently doesn't support ESM bundling.
Given that our main focus at the moment is to bring these utils to a production ready status. Marking this issue as blocked
and on-hold
till further notice.
I'm only using @aws-lambda-powertools/logger
and would really appreciate an ESM build even if aws-xray-sdk-node
is blocking @aws-lambda-powertools/tracer
. Thank you!
Recap of the status and actions required before being able to move forward with this:
While we acknowledge that there's an option to independently support ESM on some utilities while leaving others only with CJS support, we are still keen in dedicating some effort in investigating ways to bring ESM support for all utilities. We believe that doing so would result in a less confusing developer experience for users, since they wouldn't have to check which utility supports what.
At the moment, before starting to work on the feature we need to resolve the following topics:
The resolver for this issue will have to primarily investigate the scope of the changes and propose options to move forward along with considerations on the tradeoffs for each option. Ideally, each solution/option should also propose high-level examples of where the changes would be (i.e. tsconfig.json
, build system, release process, etc.), as well as detail any changes in the public API (i.e. will there be changes in how users import the libraries?).
If anyone is willing to take this on, please leave a comment below before starting to work on it.
I'm researching this
Update with my research so far. I've got some answers to the questions above from @dreamorosi. There's more I don't know yet.
Caveat: This is based on my research so far. The domain is new to me, so please point out things that seem odd.
I propose that we dual package both CommonJs and ES modules. This change would add a small size overhead to the published package, and a little extra complexity in building the packages. It shouldn't require changes to existing CI/CD pipelines. It would allow users to consume the utilities in ES Module Lambda functions, as well as CommonJS modules which are currently supported.
I think we should support both (for now). If we dual packaged both formats there would be a slight increase in package size and build complexity.
Each utility package size would increase by ~50%, as we would publish each package twice.
For example, the logger utility /lib
is 212KB ( commonjs with type declaration)
If we dual package CommonJS and ES Modules, it will increase to 344KB (132KB increase)
132KB ./lib/es
120KB ./lib/cjs
92KB ./lib/types
(separate type declaration)
Ideally would gather empirical evidence. Can't comment on this yet. I suspect the cold start would increase due to the increase package size, but it would be negligible
There is a additional complexity of creating two packages for each utility. The build process is slightly more complicated.
This proposal is based on aws-js-sdk-v3
approach. At a high level, we would generate a CommonJS output, an ES Module output and Types output.
Each output would have a seperate tsconfig
file. The base file would be for CommonJS output. The ES Module and Types outputs would extend this base config file.
tsconfig.es.json
and tsconfig.types.json
files that extends the base tsconfig.json
file. This is similar to the proposal in #617 . tsconfig
file (cjs
, es
, and types
).
lib/cjs
, lib/es
and lib/types
For each utlity, we would update the package.json
to support dual packaging. We can add the exports
property in package.json
to support both ES Modules and CommonJS. To support older versions of node, we can also include the older style module
, main
and types
properties as well. This might not be needed, I need to do more research.
tsconfig
and package.json
files. CI/CD and release would stay the same.There would be no change in how users install or use the libraries with either approach. There would be a difference in how users import the libraries.
If we supported only ESM, then users would only be able to use import the packages using import()
or import ... from ...
. They could not use require()
(commonJs syntax).
Not easily. CommonJS lambda functions could no longer use require()
to load a utility. I believe they could use the dynamic import()
function instead, but I need to do more research, and ideally a POC.
Hi @ConnorKirk thanks a lot for putting these together, it's going to be very useful and I like the approach so far.
Dual packaging seems like the way to go, at least on the surface and it seems to be in line with how most of the ecosystem is handling this.
However before making a choice it would beneficial to have more data on the actual impact on cold start and other metrics if we were in fact to dual build.
Ideally we should be able to compare the existing utilities like this on the three latest runtimes:
Once we have this data we can start and open up the discussion further.
@ConnorKirk
Thanks for the research!
@dreamorosi
I am not familiar with this domain. But I want to check that this is probably a one-way decision, right?
aws-lambda-powertools-logger-esm
and another for -cjs
. This is also not reversible. Users have to migrate for newer version.One possible thought is that we implement option 1 first (for ease of use, at the (calculated) cost of cold start), and extend to implement option 2 in our release script later if there is a demand for performance. Would that be feasible?
@ijemmy good points.
Having separate packages would be possible, however as it stands this would mean already 6 different packages only for the core utilities.
I have seen this done with different versions instead of suffix in the name, but regardless I find that it adds a bit of cognitive load.
Two additional datapoints/notes:
esbuild
should be able to tree-shake, also there are options like node-prune
. I don't have a completely formed idea about this yet, but this sounds like the kind of optimization/pruning that should be done in userland (the lib should allow it though).Aside from all this, I think we need data to understand what's the actual impact on cold-start and bundle size of the dual-bundle option.
I agree that it's not a decision we should make in a rush and take lightly. Let's wait for Connor to come back with benchmarks and then see what we are dealing with.
The task is back on the backlog.
The latest status is that currently we are inclined towards shipping both CJS & ESM and let customers do their own tree-shaking. However, before deciding we need to make an assessment and understand the impact that this will have on the deployment package, as well as understanding if the tree-shaking is actually effective.
If anyone is interested in picking this up, please leave a comment below.
This issue has not received a response in 2 weeks. If you still think there is a problem, please leave a comment to avoid the issue from automatically closing.
Hey there!
I've been working on this for some time and I am ready to share the results:
After some discussion on discord, we decided to start with the Logger
package. As the result, we should be able to ship the package with both CommonJS
and ECMAScript
modules.
Changes made to Logger
that allows building it as a package with both nodules using npm run build
.
Repo with benchmarks and results. You're welcome to adjust the app and tests, run them and share your results.
I'll go through all the changes and decisions I made.
This is a new lib
directory structure:
cjs
- CommonJS
codeesm
– ES
codetypes
– extract types
to share between both modules.To achieve this tsconfig.json
was slightly changed and tsconfig.cjs.json
, tsconfig.mjs.json
, and tsconfig.types.json
were added to transpile typescript code and put javascript code to directories above.
Configs extend tsconfig
with basically two differences module
and outDir
. For types
it only emit declarations.
Explanation of package.json
:
"type": "module", // To treat package as ES module by default
"exports": { // Node.js 12+ as an alternative to the “main”
".": {
"types": "./lib/types/index.d.ts", // Entry-point for TypeScript resolution - must occur first!
"import": "./lib/esm/index.js", // Entry-point for `import "my-package"` in ESM
"require": "./lib/cjs/index.js", // Entry-point for `require("my-package") in CJS
"default": "./lib/esm/index.js" // Default fallback
}
},
"main": "./lib/cjs/index.js", // CJS fall-back for older versions of Node.js
"module": "./lib/ems/index.js", // For build tools such as esbuild, used the ES module entry point, since Node.js ignored (and still ignores) the top-level "module" field.
"types": "./lib/types/index.d.ts", // Fall-back for older versions of TypeScript
Reference: https://www.typescriptlang.org/docs/handbook/esm-node.html https://nodejs.org/api/packages.html#dual-commonjses-module-packages
To make it work as a double bundled package. each module directory should have additional package.json
that explicitly says what kind of module it is:
cjs/package.json
with {"type":"commonjs"}
esm/package.json
with {"type":"module"}
A script that runs right after the build generates these files.
Why this way instead of using .cjs
and .mjs
file extensions?
tsc
. TypeScript compiler can produce .cjs
or .mjs
extensions, but from files with .mts
or .cts
file extensions. But we need both, as a result generating package.json
via script is a better option.Changes required to Logger codebase
Running JS code with Logger as ES module would work in node, but because of relative paths node
should be run with the --experimental-specifier-resolution=node
flag, which is dropped also in favor to custom loaders. It uses https://nodejs.org/api/esm.html#resolution-algorithm to evaluate paths.
To avoid using this flag or custom loader for our users, I followed the typescript recommendation for EMS and updated imports in the source code to make it work for both modules:
import { Logger } from “./Logger”; // only works in CJS`
import { Logger } from “./Logger.js"; // works in ESM & CJS
Reference: https://www.typescriptlang.org/docs/handbook/esm-node.html#type-in-packagejson-and-new-extensions
IMO ESM
support is worth changing imports.
These changes broke unit tests, but I fixed that with a small jest
config change:
moduleNameMapper: {
'^(\\.{1,2}/.*)\\.js$': '$1',
},
Docs: https://jestjs.io/docs/next/configuration#modulenamemapper-objectstring-string--arraystring
This regular expression matches any string that starts with one or two dots followed by a forward slash, followed by any number of characters and ends with the ".js" file extension. All the following paths will be a match:
I think I covered all the changes. If you have any questions feel free to ask.
The reason is to understand the difference between an app that used the current version of Logger from npm (1.6.0), an app with the new version of Logger build as CJS module, and an app with the new version of Logger build as ES module.
For the testing created a template for AWS SAM to build and deploy three different lambdas. Functions bundled using esbuild
and load-tested with artillery.io.
I created this repository with a description, so you can run tests yourself if you want to try other target platforms or the amount of load.
I used this article as a reference: https://aws.amazon.com/blogs/compute/optimizing-node-js-dependencies-in-aws-lambda/
The screenshot under the spoiler is the test I did today with a more extensive load for functions on the Node 16.x platform. ESM looks really good, the cold start for current
and CJS
builds is relatively the same, and warm invocations with CJS are better than Current Logger.
How to read this?
Percentile (p) indicates the relative standing of a value in a dataset. For example, p95 is the 95th percentile and means that 95 percent of the data within the period is lower than this value and 5 percent of the data is higher than this value. Percentiles help you get a better understanding of the distribution of your metric data.
Wow @shdq, thank you SO much for this huge effort and for taking the time to not only look into this but also providing a deep dive on the decision taken and a reproducible benchmark suite.
The results definitely look promising. Please allow me some time to play with the benchmarks a bit, do some tests with the new builds, and think what could be the next steps (cc @am29d).
@dreamorosi thanks for your support and encouragement!
I want to raise a discussion on the pros and cons of double packaging:
Pros
import
s the powertools package inside its app (encouraged by docs way of import tools), but has it bundled as commonJS
by mistake won't be a problem (bloated bundle). Since the library will be ESM
by default, the bundler will eliminate all the dead code and then transpile the app as CommonJS
. Only in the case whenrequire
is intentionally used in the app, tree shaking won't work, and code from lib/cjs
directory will be used for bundling. Tree shaking doesn't work with CommonJS
modules.powertools-utility
or powertools-utility-es
Prone to a mistake. You need to educate users and help them to choose the proper one. Lazy ones never discover ES
version, cause it just works. Getting an update with a double-packaged library tree shakes the dead code from their dependency without even knowing that.import
won't notice, only those who use require
will be impacted.Cons
node_modules
as is. And when you use layers (ES
is only supported in Node.js 18+), since it serves dependency for lambda in a different way (I will appreciate some help with understanding how the size impacts with layers)
Dual package hazard in node.js docs. This approach is used for double bundling.To demonstrate tree shaking, cause it doesn't have noticeable results in the previous tests, I decided to convert the Commons
package to double package, because it is Logger
dependency and it has the dead code. So it is a good opportunity to tree-shake that.
I vendored the new Logger
package to test it. I added console.log()
to the source code of both Logger
and Commons
packages, to understand that it uses proper sources during compilation by tsc
:
echo 'console.log("Log from CJS-logger package");' >> node_modules/@aws-lambda-powertools/logger/lib/cjs/index.js
echo 'console.log("Log from CJS-commons package");' >> node_modules/@aws-lambda-powertools/commons/lib/cjs/index.js
and to the ES
module
echo 'console.log("Log from ESM-logger package");' >> node_modules/@aws-lambda-powertools/logger/lib/esm/index.js
echo 'console.log("Log from ESM-commons package");' >> node_modules/@aws-lambda-powertools/commons/lib/esm/index.js
I build my app as CommonJS
with tsc
and these settings among others:
package.json
"type": "commonjs",
tsconfig.json
"module": "commonjs",
And I ran my CommonJS
app.
Then I build my app as ES
with tsc
and these settings among others:
package.json
"type": "module",
tsconfig.json
"module": "ESNext",
"moduleResolution":"node",
Then I ran my ES
app.
And it works as expected:
For tree shaking, I used the same hello world apps as in tests in the previous comment and esbuild
to create two bundles. Inside the CJS
app require
is used to import the logger, ES
app uses import
to import the logger. You can see the parameters used for esbuild
and bundle sizes on the screenshot.
What to look at in the source code?
Basically, CommonJS
has all the dead code such as samples
, pay attention to the comments paths like this:
// node_modules/aws-lambda-powertools-commons/lib/cjs/samples/resources/contexts/hello-world.js
But ES
bundle eliminated all the dead code, you can see it by the size and the actual code under the spoiler.
It also tree shake when you build an app as CJS
but with import
for logger in your app code. It is resulted in 55.3kb
(a bit more). That's why I stated that it is tree-shaking by default in the pros section.
package.json
for subdirectoriesI want to add some more info about package.json
generation with the script with module types:
When I was researching I came across this approach and tools that use it when you want to mix typescript and emit as pure .js
files for both modules. Node.js docs say that it is better to explicitly use "type" for your modules.
But it's a simple task to generate files with the script without bringing any dependency or a bundler.
I hope this additional information will help to make a better decision on how to serve the PowerTools library. If you have any questions or concerns, feel free to ask. I think we should have a section in the docs with recommendations on how to serve and tree-shake apps with PowerTools for TypeScript.
Please join the discussion on this if you use the library and have any feedback.
Hi @shdq, thank you so much for kicking things off with the initial proof of concept and also for providing such an exhaustive prospective on the implications and potential objections.
The numbers shown on Logger are promising and even though there is a slight size increase in terms of output, these are outweighed by the benefits that ESM brings and the use cases it unlocks.
This is not a decision that we are taking lightly as we know that in a compute environment like Lambda every byte counts. However we must acknowledge that without ESM, use cases like top-level async/await
are simply not possible. In addition to that, the performance benchmarks at runtime that you have shared, show that there are slight performance improvements at runtime.
As you have mentioned in the arguments in favor of this solution, tree-shaking appears to compensate for most of the size increase. For this reason, the rollout of this feature will be accompanied by new documentation around best practices for bundling and tree-shaking your functions. Additionally, it's important to mention that even though we don't have a timeline, this is a temporary solution, as the Node.js ecosystem will eventually move towards ESM and potentially deprecate CJS.
So to sum up: the proposal of using dual bundling appears a valid one and the benchmarks show positive results. We have mitigation plans for the areas that show less positive results and a plan for the rollout that we will share later on, once we have a clearer picture.
In terms of next steps, it would be ideal to see a similar experiment with benchmarks for all other Powertools utilities. This will help understand whether the new bundling will have effects in line with the ones we are seeing for Logger.
Again, thank you on behalf of myself and all users of Powertools for this amazing work Sergei!
Hello 👋
I discovered a small improvement in our take on at double packaging.
Spotify introduced their TypeScript SDK for Web API and they serve it as both ESM and CommonJS.
I looked inside and instead of generating a separate package.json
with the script as I suggested before to have both:
They simply copy it from a folder (content is the same all the time):
"build": "npm run build:cjs && npm run build:mjs",
"build:mjs": "tsc --project tsconfig.mjs.json && cp res/package.mjs.json dist/mjs/package.json",
"build:cjs": "tsc --project tsconfig.cjs.json && cp res/package.cjs.json dist/cjs/package.json",
I found it a better decision since no extra script is required.
Details: spotify-web-api-ts-sdk
@dreamorosi any updates on this issue and the next major release?
Hi @shdq apologies for the long delay in my response, I meant to give you a proper answer but failed to do so in a reasonable amount of time.
With two utilities now in public beta (Idempotency and Batch Processing), and the next ones (see Roadmap) in RFC stage we can finally turn our focus on this topic and on v2 of Powertools for AWS (TypeScript) as a whole.
In the coming weeks we'll continue the work that you have started and finalize a viable build config to add support for ESM before the end of the year.
As soon as I have more news to share I'll make sure to do so under this thread.
Hey Folks!
Just checking in on the status of this Issue. I've recently switched from using variations of Pino to using Powertools, but the lack of ESM support is hard blocker on using Powertools with ES Modules in Lambda.
I'm happy to take on the work to get shipping of dual packages. I've done it a few times before, and written about migrating from CommonJs to ES Modules.
The actual change to build and ship ES modules is pretty quick. The approach of the AWS SDK v3 team is generally accepted as best practice for shipping dual module packages.
main
field in package.json refers to the CommonJS buildmodule
field in package.json refers to the ESM buildWhere there could be challenges is with downstream dependencies that the libraries use. If any use CommonJs, it might break an ES build, the caveat being "might". Based on the experiments by @shdq I don't think thats an issue.
Let me know if you want assistance.
Ant
Hey @antstanley, if you're bundling your code, you can add a banner like import module from 'module';if (typeof globalThis.require === "undefined"){globalThis.require = module.createRequire(import.meta.url);}
to allow using CJS in ESM code. I agree that powertools should provide an ESM distribution, but that's a workaround for now.
Hi both, first of all thank you for the continued interest in this feature.
I've mentioned this in the past in a couple of places but since the discussion about the topic has been going on for a while I'd like to take a second to explain why it has been taking so long.
Even though each utility is published independently, we strive to offer a cohesive experience throughout all utilities. In the case of Tracer, which is backed by the AWS X-Ray SDK, we were unable to find a path forward due to the SDK not supporting ESM bundling.
We could have enabled ESM support for all other utilities except for Tracer, but we decided against this to avoid having features supported only by some utilities. At the same time, we made the decision to prioritize new utilities (Idempotency & Batch Processing) over this feature also hoping that the situation upstream would change.
The X-Ray SDK still doesn't support ESM bundling, however the workaround proposed by @bestickley seems promising and I was able to run a function that uses the current Tracer & X-Ray SDK bundled as ESM with esbuild
. Thanks to this I think we can finally move forward with the topic.
To manage expectations, we still plan on releasing ESM support as part of a v2 release. The reasoning behind this is that due to CJS+ESM dual bundling we might have to change some of the imports/exports thus increasing the likelihood of a breaking change.
We committed to include three areas in our v2 release (see roadmap):
feat/v2
branch (Logging providers)@antstanley thank you for the generous offer, we would be incredibly happy to accept your offer 🎉
Based on your experience, do you think it'd be possible to approach this conversion one utility at the time? Our codebase is a monorepo but each utility (under packages/*
) has its own set of tsconfig.json
files. We already have a v2 branch so my initial suggestion would be to have each utility in a separate PR.
This would help with keep the blast radius contained, have faster reviews, and also allow us to potentially work on replicating the changes from the first PR to other utility in parallel.
What do you think?
Hey folks! Apologies for going quiet on this. Had other things come up. I've got time now to work on this. I'll do a PR on the v2 branch with a potential way to implement this. Doing a little more investigation using the exports
property of package.json
to implement conditional exports could be a good way to do this vs main
and module
field, (or maybe do all to satisfy all bundlers/environments)
Conditional Exports: https://nodejs.org/api/packages.html#conditional-exports
Hi @antstanley, thanks for the offer and don't worry about the timing.
I have already started working on this and have a working branch where I was able to implement this the hybrid bundling on at least two of the modules (commons & tracer).
I have a couple of issues to fix but after that I should be able to open a PR, which I expect to complete this week.
Would you be open to help review the PR instead?
@dreamorosi Github notifications are letting me down today! I've created a PR here #1734 , forked from your v2
branch.
As in the PR description, it's there mainly as a discussion point.
It fully works, and all tests pass. It's also isolated, so can use that approach on a package by package basis. It also doesn't require commons
to have an ESM build.
Probably spent more time fixing the tests and getting the jest
/ ts-jest
config right.
All the modules have now been converted to esm + cjs on the feat/v2
branch.
In the next days I'll work on creating an alpha release channel so that you can start testing the new version and let us know if there's any issue with the way things are built.
Once again, a huge thank you to everyone who interacted with the issue and helped push this forward - especially @shdq and @antstanley.
Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.
Description of the feature request
Problem statement Powertools is packaged as commonJS which does not give the capability to javascript developers to use esmodule import syntax and feature.
Summary of the feature Now that AWS Lambda support es module import it might worth generating esmodule.
Code examples
Before:
After:
Benefits for you and the wider AWS community See https://aws.amazon.com/blogs/compute/using-node-js-es-modules-and-top-level-await-in-aws-lambda/ for more details ...
Additional context slack channel discussion
Related issues, RFCs
https://github.com/awslabs/aws-lambda-powertools-typescript/issues/490