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

ESBuild doesn't tree shake *Client imports #3542

Closed
trivikr opened this issue Apr 14, 2022 · 8 comments
Closed

ESBuild doesn't tree shake *Client imports #3542

trivikr opened this issue Apr 14, 2022 · 8 comments
Labels
guidance General information and guidance, answers to FAQs, or recommended best practices/resources.

Comments

@trivikr
Copy link
Member

trivikr commented Apr 14, 2022

Describe the bug

ESBuild doesn't tree shake *Client imports

This is probably an issue with esbuild or my configuration. I'm creating a bug report with minimal repro.

Your environment

SDK version number

@aws-sdk/client-acm@3.67.0

Is the issue in the browser/Node.js/ReactNative?

Node.js

Details of the browser/Node.js/ReactNative version

$ node -v
v16.14.2

$ ./node_modules/.bin/esbuild --version
0.14.36

Steps to reproduce

// bare-bones-client.mjs
import { ACMClient, ListCertificatesCommand } from "@aws-sdk/client-acm";

export const listCertificates = () => {
  const client = new ACMClient({ region: "us-west-2" });
  return client.send(new ListCertificatesCommand({}));
};
// aggregated-client.mjs
import { ACM } from "@aws-sdk/client-acm";

export const listCertificates = () => {
  const client = new ACM({ region: "us-west-2" });
  return client.listCertificates({});
};

Observed behavior

$ ./node_modules/.bin/esbuild bare-bones-client.mjs --bundle --outfile=out.js --platform=node 
  out.js  750.0kb
$ ./node_modules/.bin/esbuild aggregated-client.mjs --bundle --outfile=out.js --platform=node 
  out.js  749.9kb

Expected behavior

I was expecting the bundle created from bare-bones-client.mjs to be much smaller.

@trivikr trivikr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 14, 2022
@trivikr
Copy link
Member Author

trivikr commented Apr 14, 2022

This is probably an issue with esbuild or some configuration, as it's present in @aws-sdk/client-acm@3.0.0 too.

$ ./node_modules/.bin/esbuild bare-bones-client.mjs --bundle --outfile=out.js --platform=node
  out.js  542.4kb

$ ./node_modules/.bin/esbuild aggregated-client.mjs --bundle --outfile=out.js --platform=node
  out.js  542.4kb

@trivikr
Copy link
Member Author

trivikr commented Apr 14, 2022

Probably related evanw/esbuild#1420

@AllanZhengYP
Copy link
Contributor

I don't see you supplied a tree-shake config, here is the esbuild config I used for our bundler test.

@trivikr
Copy link
Member Author

trivikr commented Apr 14, 2022

From esbuild docs on tree shaking, it's enabled when bundling

By default, tree shaking is only enabled either when bundling is enabled or when the output format is set to iife, otherwise tree shaking is disabled.

I force-enabled tree-shaking by setting --tree-shaking=true, and verified that bundle size doesn't change:

$ ./node_modules/.bin/esbuild bare-bones-client.mjs --bundle --outfile=out.js --platform=node --tree-shaking=true
  out.js  750.0kb

$ ./node_modules/.bin/esbuild aggregated-client.mjs --bundle --outfile=out.js --platform=node --tree-shaking=true
  out.mjs  749.9kb 

@trivikr
Copy link
Member Author

trivikr commented Apr 14, 2022

Experimented with cjs code, but there's no difference:

// bare-bones-client.js
const { ACMClient, ListCertificatesCommand } = require("@aws-sdk/client-acm");

export const listCertificates = () => {
  const client = new ACMClient({ region: "us-west-2" });
  return client.send(new ListCertificatesCommand({}));
};
// aggregated-client.js
const { ACM } = require("@aws-sdk/client-acm");

export const listCertificates = () => {
  const client = new ACM({ region: "us-west-2" });
  return client.listCertificates({});
};
$ ./node_modules/.bin/esbuild bare-bones-client.js --bundle --outfile=out.js --platform=node 
  out.js  749.7kb

$ ./node_modules/.bin/esbuild aggregated-client.js --bundle --outfile=out.js --platform=node
  out.js  749.6kb

@trivikr
Copy link
Member Author

trivikr commented Apr 14, 2022

This happens as esbuild can't tree shake commonjs modules

This means tree shaking will likely not happen for packages that provide both module and main since tree shaking works with ECMAScript modules but not with CommonJS modules.

The application can explicitly pass --main-fields=module,main to enable tree-shaking

$ ./node_modules/.bin/esbuild aggregated-client.mjs --bundle --outfile=out.js --platform=node --main-fields=module,main
  out.js  591.5kb

$ ./node_modules/.bin/esbuild bare-bones-client.mjs --bundle --outfile=out.js --platform=node --main-fields=module,main
  out.js  486.9kb

@trivikr trivikr added guidance General information and guidance, answers to FAQs, or recommended best practices/resources. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 14, 2022
@trivikr trivikr closed this as completed Apr 14, 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 Apr 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
guidance General information and guidance, answers to FAQs, or recommended best practices/resources.
Projects
None yet
Development

No branches or pull requests

2 participants