Not treeshaking && variables

[Gusted]

Hey!

So, I've recently hacked around my project dark reader to switch from rollup to ESBuild and try to lose no features over rollup other than it's slow bundling, the only thing that is missing is instanbul's code-coverage alongside karma but that isn't the point now.

So we use 2 functions logWarn and logInfo to output debug information when the DEBUG variable is true, we currently do this dynamically by defining __DEBUG__ to the debug flag in our bundling process. Rollup always removed any instances to those 2 function when the DEBUG variable was false. As it wouldn't run anyway.

However, I've noticed with ESBuild's treeshaking this isn't the case and no dead code elimination seems to be done while treeshaking is enabled. As I'm following this project for a while I know that a simple repo will come in handy https://github.com/Gusted/esbuild-declare/ a simple and clean repo. At first glance I thought that defining variables didn't work with the dead code elimination and also created a second build for -correct that didn't implement any defining however the result was the same and no dead code elimination was in place.

As rollup will be a big hassle to implement with typescript compiling it wasn't added, but this was rather for a comparison that Rollup does this.

Rollup treeshakes it down to outputting nothing, or when other relevant code that will run is added. Minifying the functions to

    function logInfo(...args) {}
    function logWarn(...args) {}

And removing the DEBUG variable and leave all calls to logInfo/logWarn intact.

Esbuild currently doesn't do anything and will output:

(() => {
  // debug.ts
  var DEBUG = false;
  function logInfo(...args) {
    DEBUG && console.info(...args);
  }

  // index.ts
  logInfo("Media query was changed");
})();

Even though at first glance you can see nothing will ever happen other than evaluating some variable.

Regards,
Gusted

[evanw]

That's correct, esbuild won't remove this as dead code. That's because it doesn't currently do constant detection and inlining like this. I haven't done this yet because doing that is actually pretty hard and can sometimes be impossible. For example:

// index.ts
logInfo("Media query was changed");

// debug.ts
var DEBUG = true;
function logInfo(...args) {
  DEBUG && console.info(...args);
}

In this case it would be incorrect for esbuild to replace DEBUG with true because that variable isn't initialized yet and has the value undefined. While you could handle this case by tracing the call graph, that doesn't work if parts of the call graph aren't statically-analyzable (for example if part of the call graph depends on a property access). So it can't always be done in the general case.

Instead, the intended way to do this with esbuild is with the define feature. Remove the DEBUG variable declaration and have it reference a global variable instead. Then use esbuild's define feature to replace all occurrences of DEBUG with true or false. This is easier for esbuild because having it be a global means that it can be considered to already be initialized by the time the code runs.

[Gusted]

In this case it would be incorrect for esbuild to replace DEBUG with true because that variable isn't initialized yet and has the value undefined. While you could handle this case by tracing the call graph, that doesn't work if parts of the call graph aren't statically-analyzable (for example if part of the call graph depends on a property access). So it can't always be done in the general case.

Mhm, I understand mutable variables are indeed hard to parse. Not sure why Rollup then removed it, best assumption is that they don't replace the top-level variables to var and still have

const DEBUG = false;

Which is immutable and therefore safe assume to remove it.

Instead, the intended way to do this with esbuild is with the define feature. Remove the DEBUG variable declaration and have it reference a global variable instead. Then use esbuild's define feature to replace all occurrences of DEBUG with true or false. This is easier for esbuild because having it be a global means that it can be considered to already be initialized by the time the code runs.

I've tried it and that indeed works! Thanks for your explanation!

So I had little question, not sure if you want it into a new issue or not so.
I saw the Safari Performance issue and the new option avoidTDZ, did this change make every top-level variable to var. As with both enabled and disabled the top-level variables still compile to var.

Regards,
Gusted

[evanw]

Which is immutable and therefore safe assume to remove it.

Accessing this variable before it's defined is technically supposed to throw an exception, so the behavior is still different than replacing it with false. The fact that uses the const keyword doesn't really help. It's pretty trivial to tell if a let variable has no assignments and is therefore a constant. The hard part with inlining variables is what to do about uninitialized variables, not with determining whether it's constant or not.

I saw the Safari Performance issue and the new option avoidTDZ, did this change make every top-level variable to var. As with both enabled and disabled the top-level variables still compile to var.

The avoidTDZ option is now always enabled and cannot be disabled. The option itself no longer does anything and will be removed in the next round of breaking changes. I added the option because the transformation wasn't totally safe in all cases, but then later found ways of making it safe so having to opt-in is no longer needed.

This is force-enabled both because of the Safari performance issue and because of some upcoming changes to the bundler. I'm planning on making modules lazily-initialized in some cases related to code splitting. To do that and still keep the performance benefits of ESM's static binding semantics, you need to defer the initialization of top-level constants. That means they can't be const anymore.

There is also a benefit for minification, not just because var is shorter than const but also because changing all of the top-level variable declarations to var means adjacent variable statements can be merged together into a single declaration even if one is const and the other one isn't. Assignments to const symbols are prevented at compile time so there shouldn't be a run-time behavior difference.

[Gusted]

Accessing this variable before it's defined is technically supposed to throw an exception, so the behavior is still different than replacing it with false. The fact that uses the const keyword doesn't really help. It's pretty trivial to tell if a let variable has no assignments and is therefore a constant. The hard part with inlining variables is what to do about uninitialized variables, not with determining whether it's constant or not.

Aha, I understand. That actually make sense now thinking about it.

The avoidTDZ option is now always enabled and cannot be disabled. The option itself no longer does anything and will be removed in the next round of breaking changes. I added the option because the transformation wasn't totally safe in all cases, but then later found ways of making it safe so having to opt-in is no longer needed.

This is force-enabled both because of the Safari performance issue and because of some upcoming changes to the bundler. I'm planning on making modules lazily-initialized in some cases related to code splitting. To do that and still keep the performance benefits of ESM's static binding semantics, you need to defer the initialization of top-level constants. That means they can't be const anymore.

There is also a benefit for minification, not just because var is shorter than const but also because changing all of the top-level variable declarations to var means adjacent variable statements can be merged together into a single declaration even if one is const and the other one isn't. Assignments to const symbols are prevented at compile time so there shouldn't be a run-time behavior difference.

I'm looking forward to see those changes and that is some great logic you explained here.

Thank you for clearing certain things up. I'm looking to see the changes you mentioned coming into ESBuild and thanks for the project!

Regards,
Gusted