Skip to content

A Node.js application written in ESM uses CommonJS distribution of the SDK #2900

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

Closed
trivikr opened this issue Oct 14, 2021 · 10 comments
Closed
Labels
feature-request New feature or enhancement. May require GitHub community feedback.

Comments

@trivikr
Copy link
Member

trivikr commented Oct 14, 2021

Is your feature request related to a problem? Please describe.

A Node.js application written in ESM uses CommonJS distribution of the SDK.

Example code:

// index.mjs
import { DynamoDB } from "@aws-sdk/client-dynamodb";

const client = new DynamoDB({});
await client.listTables({});
  • In VSCode, add a breakpoint on the line which calls listTables.
  • Click on "Run and Debug" in the Debugging View to start debugging.
  • The debug session will break at listTables.
  • Click on "Step Into" debug action.
  • Note that debug session steps into node_modules/@aws-sdk/property-provider/dist-cjs/chain.js

It uses distribution in dist-cjs and not in dist-es

Screen recording
StepThroughMJS.mov

Describe the solution you'd like

The Node.js application written in ESM should use distribution in dist-es
The Node.js application written in ESM should use folder in dist-node-esm (UPDATE April 2022)

Describe alternatives you've considered

A different distribution for Node.js ESM, which can be take up while implementing Node.js specific distributions in #2889 to avoid increasing install size.

Additional context

@trivikr trivikr added the feature-request New feature or enhancement. May require GitHub community feedback. label Oct 14, 2021
@simonbuchan
Copy link
Contributor

This one should actually be pretty easy to fix, assuming es doesn't assume browser: the current package.json files have:

  "main": "./dist-cjs/index.js",
  "module": "./dist-es/index.js",

which was the bundler convention, but the native Node ESM support ignores module, and instead uses the new exports specification, and in particular conditional exports, so simply adding the following should work:

  "exports": {
    "require": "./dist-cjs/index.js",
    "import": "./dist-es/index.js",
  },

Note that this will enforce encapsulation, so anybody doing something like import "@aws-sdk/client-s3/lib/some-file.js" will be broken... but you probably want that.

@tenjaa
Copy link

tenjaa commented Feb 17, 2022

I am trying to use aws-cdk with the nodejs function. This bundles my lambda code with esbuild.
Using top-level await forces me to use ESM as target, which did not work and lead me to this issue.
Do you know a workaround for the time being?

@trivikr
Copy link
Member Author

trivikr commented Apr 11, 2022

Do you know a workaround for the time being?

@tenjaa Can you create a new bug report?

@trivikr
Copy link
Member Author

trivikr commented Apr 11, 2022

Changes needed if (and when) we publish Node.js (ESM) artifacts in future.

  "main": "./dist-node-cjs/index.js",
  "module": "./dist-node-esm/index.js",
  "exports": {
    "require": "./dist-node-cjs/index.js",
    "import": "./dist-node-esm/index.js",
  },

The naming is picked up from suggestions in #3531

@trivikr
Copy link
Member Author

trivikr commented Apr 12, 2022

Node.js official docs recommend two solutions for dual-packaging: An ESM wrapper or Isolate State.

On quick look, the ESM wrapper appears to be a better solution as:

  • The install size is expected to increase just a little because of wrapper code. Emitting distinct ESM artifacts will double the install size for Node.js variant.
  • The build time will not increase significantly, as we just need to create a wrapper. There are existing community supported utilities like gen-esm-wrapper. Generating distinct ESM artifacts will require a new run of TypeScript compiler.
  • The maintenance burden will be low in ESM wrapper solution. We don't have to manage another TSConfig/dist folder/build script.

If we decide to use ESM Wrapper, the package.json would be as follows:

  "main": "./dist-node/index.js",
  "module": "./dist-node/index.mjs",
  "exports": {
    "require": "./dist-node/index.js",
    "import": "./dist-node/index.mjs",
  },

@simonbuchan
Copy link
Contributor

Note that those recommendations are largely for the case where you have globalish state that could cause problems when there are multiple versions bundled. So far as I've seen messing around in the internals so far, this repo mostly just has caches and the like, nothing like the v2 global config.

That said, so long as you don't need to manage the wrappers, that should work fine. I've had lots of headaches managing multiple builds.

The real question though, is if you actually need a commonjs build at all at this point? You're either running directly in node or though a bundler, in both cases you have native ESM support in their supported versions: node v12.22.0 stabilized esm support, and that's the oldest supported version.

@simonbuchan
Copy link
Contributor

Ah, of course, you can't require() a module.

@skyrpex
Copy link

skyrpex commented Apr 24, 2022

Node.js official docs recommend two solutions for dual-packaging: An ESM wrapper or Isolate State.

On quick look, the ESM wrapper appears to be a better solution as:

  • The install size is expected to increase just a little because of wrapper code. Emitting distinct ESM artifacts will double the install size for Node.js variant.
  • The build time will not increase significantly, as we just need to create a wrapper. There are existing community supported utilities like gen-esm-wrapper. Generating distinct ESM artifacts will require a new run of TypeScript compiler.
  • The maintenance burden will be low in ESM wrapper solution. We don't have to manage another TSConfig/dist folder/build script.

If we decide to use ESM Wrapper, the package.json would be as follows:

  "main": "./dist-node/index.js",
  "module": "./dist-node/index.mjs",
  "exports": {
    "require": "./dist-node/index.js",
    "import": "./dist-node/index.mjs",
  },

Yup, it's important to use either .cjs or .mjs file extensions, otherwise one of the bundles will be broken (depending on whether the library's package.json has a "type":"module" or not in it.

@trivikr
Copy link
Member Author

trivikr commented Oct 13, 2022

Closing this in favor of #3223

@trivikr trivikr closed this as completed Oct 13, 2022
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request New feature or enhancement. May require GitHub community feedback.
Projects
None yet
Development

No branches or pull requests

4 participants