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

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

Closed
ghiscoding opened this issue Jun 14, 2023 · 5 comments
Closed

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

ghiscoding opened this issue Jun 14, 2023 · 5 comments

Comments

@ghiscoding
Copy link

ghiscoding commented Jun 14, 2023

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
Copy link

hyrious commented Jun 15, 2023

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
Copy link
Author

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
Copy link
Author

@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
Copy link
Owner

evanw commented Jun 16, 2023

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
Copy link
Author

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

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