evanw / esbuild

An extremely fast bundler for the web
https://esbuild.github.io/
MIT License
38.24k stars 1.16k forks source link

code not being tree shaked in ESM build when nullifying object #3162

Closed ghiscoding closed 1 year ago

ghiscoding commented 1 year ago

We have this old open source project SlickGrid used by thousand of users, I'm slowly rewriting the code to support 3 build formats ifie, cjs and esm and I'm using this plugin esbuild-plugin-ifdef to execute conditional code while building for ESM. The project was originally built as iife and I still want to support that standalone format, by having a global name of Slick on the window object, while also produce ESM build

Repro

Here's a https://esbuild.github.io/try repro

Code repro

// index.ts
import { Utils as Utils_ } from './utils.ts'; // dropped in iife by custom plugin of mine

//#ifdef ESM_ONLY
window.Slick = null; // enabled only for ESM build
//#endif

// use this ternary as a trick so that I can build for both iife and esm
// for iife, this is great I got no dead code because I have a custom plugin that removes all imports
// but for esm build I get window.Slick dead code not being tree shaked 
const Utils = window.Slick ? Slick.Utils : Utils_; 

// use utils
Utils.createDomElement('div', { className: 'header' }); 
// utils.ts
export function createDomElement(tagName, elementOptions) {
    const elm = document.createElement(tagName);

    if (elementOptions) {
      Object.keys(elementOptions).forEach((elmOptionKey) => {
        const elmValue = elementOptions[elmOptionKey];
        if (typeof elmValue === 'object') {
          Object.assign(elm[elmOptionKey], elmValue);
        } else {
          elm[elmOptionKey] = (elementOptions)[elmOptionKey];
        }
      });
    }
    return elm;
}

export const Utils = {
    createDomElement
}

when running with this config

{
  bundle: true,
  minifySyntax: true,
  format: 'esm',
  treeShaking: true,
  outfile: 'index.js'
}

Expectation

I was expecting all window.Slick code to be dropped with tree skaking process (even with /* @__PURE__ */ it makes no difference)

So I was expecting this code

import { Utils as Utils_ } from './utils.ts'; // dropped in iife by custom plugin of mine
window.Slick = null; // for ESM only
const Utils = window.Slick ? Slick.Utils : Utils_;
Utils.createDomElement('div', { className: 'header' }); 

to be transformed to this code

import { Utils } from './utils.ts';
Utils.createDomElement('div', { className: 'header' }); 

but in reality the output is

window.Slick = null;
var Utils2 = (
  window.Slick ? Slick.Utils : Utils
);
Utils2.createDomElement("div", { className: "header" });

Technically speaking window.Slick is null so the first ternary case window.Slick ? Slick.Utils will never be reached, so why isn't it removed? Is there another way to get it removed somehow?

For now I can live with what esbuild creates but it would be nice if I could somehow delete this unreachable code for my ESM output and lower my build size a little. If it's not possible, I can live with what I have about 100 lines like this, and everything is working fine (it's just dead code that's all).

hyrious commented 1 year ago

The reason behind is that window.Slick is maybe a getter/setter which involves side effects. Considering your code run after these code:

Object.defineProperty(window, 'Slick', {
  get: () => {
    console.log('get Slick'); return 1
  },
  set: (v) => {
    console.log('set Slick')
  }
})

So esbuild cannot remove codes with side effects.

/* @__PURE__ */ doesn't help here because that annotation is for callings. i.e.

var a = /* @__PURE__ */ foo(whatever)

Is there another way to get it removed somehow?

Generally speaking, esbuild does not do too much analyze on your source code, it can only handle trivial cases which often occurs only in one AST node or one file. esbuild.try link

// use --define:ESM_ONLY=true to toggle behavior
import { Utils as Utils_ } from './utils.ts';

let Utils
if (ESM_ONLY) {
  window.Slick = null
  Utils = Utils_
} else {
  Utils = Slick.Utils
}

// use utils
Utils.createDomElement('div', { className: 'header' }); 
ghiscoding commented 1 year ago

oh nice thanks for the alternative, I didn't know about the define option, this seems to do exactly what I was looking for and even support direct condition check in the code instead of comments like I was doing with the external ifdef plugin. So this seems like a much better approach to remove code conditionally. I'll close the issue then, thanks a lot

ghiscoding commented 1 year ago

@hyrious quick question, is there a reason why it doesn't work as regular boolean? I get this error, which can also be seen in the esbuild try you provided

Expected value for define "ESM_ONLY" to be a string, got boolean instead

The readme seems to only show docs for the CLI, but I see boolean is mentioned so...

Replacement expressions must either be a JSON object (null, boolean, number, string, array, or object) or a single identifier.

Apart from that, it works great and I no longer need the external ifdef plugin which is awesome

evanw commented 1 year ago

Look at the example JS code right above the line you quoted (click "JS" in the "CLI | JS | Go" switcher at the top-right corner of the example code):

> import * as esbuild from 'esbuild'

> let js = 'hooks = DEBUG && require("hooks")'

> (await esbuild.transform(js, {
    define: { DEBUG: 'true' },
  })).code
'hooks = require("hooks");\n'

> (await esbuild.transform(js, {
    define: { DEBUG: 'false' },
  })).code
'hooks = false;\n'

It demonstrates how to pass a boolean to define. Each entry in the define map maps an identifier to a string containing the code to replace it with (i.e. the replacement expression).

ghiscoding commented 1 year ago

dohhh I feel stupid, I didn't think on ever clicking these buttons. So from what I can see, boolean must be stringified. Alright then, thanks a lot and that concludes my questions