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

Dead code removal #2006

Closed
Prinzhorn opened this issue Feb 9, 2022 · 6 comments
Closed

Dead code removal #2006

Prinzhorn opened this issue Feb 9, 2022 · 6 comments

Comments

@Prinzhorn
Copy link

I'm sorry I could not come up with a better title. I'm aware of #1317 and I think what I'm trying to achieve is very similar but it does not appear to properly remove the dead code but it gets very close.

I want to exclude certain functionality from a "free" version of my bundle in contrast to the paid "pro" version. This is controlled by process.env.FLAVOR which I've put in flavor.js. The simplified demo looks like this:

flavor.js

export const isProFlavor = process.env.FLAVOR === 'pro';

index.js

import { isProFlavor } from './flavor.js';

if(isProFlavor) {
  console.log('Pro');
} else {
  console.log('Free');
}

Using 0.14.21

esbuild index.js --bundle --minify --format=esm --define:process.env.FLAVOR=\'pro\' --outfile=out.js

Produces

console.log(!0?"Pro":"Free");

So esbuild is smart enough to inline the const expression ('pro' === 'pro') and even remove the entire if-block. But it was not smart enough to just remove the entire else branch instead of combining both branches. So I'm kind of impressed that it turned this into a single console.log, but the expected output would be

console.log("Pro");

This test case produces the desired output:

const isProFlavor = process.env.FLAVOR === 'pro';

if(isProFlavor) {
  console.log('Pro');
} else {
  console.log('Free');
}

If I understand your comment (#1981 (comment)) correctly, my flavor.js should work?

This optimization would break if you deviate from that pattern but that doesn’t seem too bad because you could just move the constants to a separate file with nothing else in it and add a comment saying not to deviate from the pattern.

It has nothing but a single const.

@evanw
Copy link
Owner

evanw commented Feb 9, 2022

The bundling pipeline is designed for speed, not for optimal output. It currently looks like this:

  1. For each input file in parallel
    1. Parse the file
    2. Minify the file (inline file-local constants, remove dead code branches)
  2. Link all files together
    1. Do tree shaking to remove unused code
  3. For each output file in parallel
    1. Do cross-module constant inlining by replacing constants with their values
    2. Print the file

As you can see, the cross-module inlining happens during printing which is very late in the pipeline. It's after tree shaking so it won't be able to affect tree shaking. It's basically a specialized hack and isn't a general-purpose approach to optimization. So for example while console.log(!1?pro():free()) could be made to be printed as console.log(free()) instead, the definition of pro would still be in the bundle (assuming the !1 was a cross-module inlined constant).

What you're asking for is not a good fit for esbuild's pipeline as it currently exists. It's possible to build a tool that does what you're asking for while still being fast, but it would require a different architecture which would involve overhauling most of esbuild, which is likely just not going to happen.

If you want esbuild to do what you're asking, then you could either:

  • Use --define:isProFlavor=false or --define:isProFlavor=true instead of importing a constant:

    if(isProFlavor) {
      console.log('Pro');
    } else {
      console.log('Free');
    }

    This works because define is run during parsing which comes before tree shaking.

  • Run esbuild twice. The second call should be able to do further optimizations. You'd probably want to make sure the intermediate output is in the esm format so tree-shaking can still work.

  • Use a different tool instead.

I'm kind of impressed that it turned this into a single console.log

This actually happens during the first stage when the file is being parsed and minified. At that point esbuild doesn't know anything about isProFlavor but it knows that console.log has been marked as side-effect free in its internal database, which means esbuild knows it can reorder console.log before isProFlavor.

@Prinzhorn
Copy link
Author

Thanks a lot for the explanation. Using --define:isProFlavor=true sounds like a practical approach.

@jimmywarting
Copy link

hmm, kind of hope that it would eliminate code such as:

false ? console.log(1) : console.log(2)

or

const isDev = false
isDev ? console.log(1) : console.log(2)

but it didn't

@evanw
Copy link
Owner

evanw commented Jan 19, 2024

Turning on minification removes the false branch: (link). You can also explicitly strip console.log calls if that's what you're after: (link). Which can be combined with minification to remove everything: (link).

@jimmywarting
Copy link

jimmywarting commented Jan 19, 2024

Hmm, you where right... i was more looking at why some part of my code was still there.
i had code similar to:

function deadCode () {
  console.log('dead code')
}

function isUsed () {
  console.log('hello world')
}

false ? deadCode() : isUsed()

export {}

i expected deadCode to be removed...

it's more or less transformed to:

// unused
function deadCode () {
  console.log('dead code')
}

function isUsed () {
  console.log('hello world')
}

isUsed()

export {}

@evanw
Copy link
Owner

evanw commented Jan 19, 2024

Here's an example of that working fine too: (link)

If you don't enable bundling then esbuild builds in a mode where by default it assumes that you might append additional code to its output (i.e. that the input is a partial module instead of a complete module) and that additional code might need to reference top-level variables in the input. This assumption was added because people actually do this, and so esbuild stripping dead code by default might be unsafe. The --tree-shaking=true option removes that assumption which allows deadCode to be tree-shaken.

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

No branches or pull requests

3 participants