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

Import gets tree-shaken despite looking like it's being protected by sideEffects #2465

Closed
2-5 opened this issue Aug 13, 2022 · 7 comments
Closed

Comments

@2-5
Copy link

2-5 commented Aug 13, 2022

This import gets wrongly tree shaken, together with it's side effects, by esbuild 0.14.54, used through vite 3.0.7:

import "@interactjs/auto-start"

interact.js docs showing above import is the way they recommend usage (see sidebar):
https://interactjs.io/docs/installation/#npm-streamlined

Installed node_modules/@interactjs/auto-start/index.js looks like this:

import interact from "../interact/index.js"
import plugin from "./plugin.js"
interact.use(plugin);

Installed node_modules/@interactjs/auto-start/package.json looks like this (simplified):

{
  "name": "@interactjs/auto-scroll",
  "version": "1.10.17",
  "main": "index",
  "module": "index",
  "sideEffects": [
    "**/index.js",
    "**/index.prod.js"
  ]
}

vite config, from https://vitejs.dev/guide/migration.html#experimental

build: {
  target: "esnext",
  commonjsOptions: { include: [] },
},

optimizeDeps: {
  disabled: false,
},

As a workaround I did this, but it's not documented anywhere on their site, so it might break in the future:

import autoStartPlugin from "@interactjs/auto-start/plugin"
import interact from "@interactjs/interact"
interact.use(autoStartPlugin)
@2-5 2-5 changed the title Import gets tree-shaken despite looking like itbeing protected by sideEffects Import gets tree-shaken despite looking like it's being protected by sideEffects Aug 13, 2022
@hyrious
Copy link

hyrious commented Aug 13, 2022

It does not happen on bare esbuild:

> echo 'import "@interactjs/auto-start"' | esbuild --bundle
[WARNING] Ignoring this import because "node_modules/@interactjs/auto-start/index.js" was marked as having no side effects [ignored-bare-import]

    <stdin>:1:7:
      1  import "@interactjs/auto-start";
                ~~~~~~~~~~~~~~~~~~~~~~~~

  It was excluded from the "sideEffects" array in the enclosing "package.json" file

    node_modules/@interactjs/auto-start/package.json:21:2:
      21    "sideEffects": [
            ~~~~~~~~~~~~~

1 warning
(() => {
})();

I think this should be a Vite-specific issue, as they have a special optimize step which must bundle third-party libraries without solving side effects' hints. A possible solution was not using the optimized modules, but instead bundle the source directly in their build step (which might slows down the build process). Anyway, this is not an esbuild bug.

Update: It seems both a bug in Vite and rollup (see comments in the vite issue).

@2-5
Copy link
Author

2-5 commented Aug 13, 2022

I'm a bit confused, doesn't your command line invocation actually demonstrate the problem? Because esbuild outputs nothing.

If instead I invoke esbuild with --ignore-annotations the code is included:

> echo 'import "@interactjs/auto-start"' | esbuild --bundle --ignore-annotations
(() => {
  // node_modules/@interactjs/utils/domObjects.js
  var domObjects = {
    init,
    document: null,

  };

  // ...
  // ...
  // ...

  // node_modules/@interactjs/auto-start/plugin.js
  var plugin_default = {
    id: "auto-start",
    install(scope2) {
      scope2.usePlugin(base_default);
      scope2.usePlugin(hold_default);
      scope2.usePlugin(dragAxis_default);
    }
  };

  // node_modules/@interactjs/auto-start/index.js
  interact_default.use(plugin_default);
})();

@hyrious
Copy link

hyrious commented Aug 13, 2022

Oops, yes. So it is both a bug in 3 tools :-/

@2-5
Copy link
Author

2-5 commented Aug 13, 2022

I'm not sure it's related to that other bug. If I bundle with rollup - by commenting out the experimental vite support for bundling with esbuild - the code does not get dropped, so it works correctly in vite with rollup bundling.

@evanw
Copy link
Owner

evanw commented Aug 13, 2022

Can someone describe how to reproduce the problem? And describe what isn't expected about the output? Here's what I did:

$ npm init -y
$ npm i esbuild@0.15.2 @interactjs/auto-start@1.10.17
$ echo 'import "@interactjs/auto-start"' | ./node_modules/.bin/esbuild --bundle

I get a lot of output that looks like this (and there are no warnings):

(() => {
  // node_modules/@interactjs/utils/domObjects.js
  var domObjects = {
    init,
    document: null,
    DocumentFragment: null,
    SVGElement: null,
    SVGSVGElement: null,
    SVGElementInstance: null,
    Element: null,
    HTMLElement: null,
    Event: null,
    Touch: null,
    PointerEvent: null
  };
  function blank() {
  }
  var domObjects_default = domObjects;
  function init(window3) {
    const win2 = window3;
    domObjects.document = win2.document;
    domObjects.DocumentFragment = win2.DocumentFragment || blank;
    domObjects.SVGElement = win2.SVGElement || blank;
    domObjects.SVGSVGElement = win2.SVGSVGElement || blank;
    domObjects.SVGElementInstance = win2.SVGElementInstance || blank;
    domObjects.Element = win2.Element || blank;
    domObjects.HTMLElement = win2.HTMLElement || domObjects.Element;
    domObjects.Event = win2.Event;
    domObjects.Touch = win2.Touch || blank;
    domObjects.PointerEvent = win2.PointerEvent || win2.MSPointerEvent;
  }

  // node_modules/@interactjs/utils/isWindow.js
  var isWindow_default = (thing) => !!(thing && thing.Window) && thing instanceof thing.Window;

...

  // node_modules/@interactjs/auto-start/plugin.js
  var plugin_default = {
    id: "auto-start",
    install(scope2) {
      scope2.usePlugin(base_default);
      scope2.usePlugin(hold_default);
      scope2.usePlugin(dragAxis_default);
    }
  };

  // node_modules/@interactjs/auto-start/index.js
  interact_default.use(plugin_default);
})();

I would need both the exact input files and the esbuild command arguments used to be able to reproduce and fix this.

@2-5
Copy link
Author

2-5 commented Aug 13, 2022

I think it's a Windows only bug. I just tried your steps, and they fail on my Windows 10 x64, but work on my Ubuntu 22 where I get output matching yours.

Maybe the / in sideEffects is checked against \ on Windows?

"sideEffects": [
    "**/index.js",
    "**/index.prod.js",
]
D:\temp>npm init -y
Wrote to D:\temp\package.json:

{
  "name": "temp",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC"
}



D:\temp>npm i esbuild@0.15.2 @interactjs/auto-start@1.10.17

added 7 packages, and audited 8 packages in 8s

found 0 vulnerabilities

D:\temp>echo import "@interactjs/auto-start" | .\node_modules\.bin\esbuild --bundle
▲ [WARNING] Ignoring this import because "node_modules/@interactjs/auto-start/index.js" was marked as having no side effects [ignored-bare-import]

    <stdin>:1:7:
      1 │ import "@interactjs/auto-start"
        ╵        ~~~~~~~~~~~~~~~~~~~~~~~~

  It was excluded from the "sideEffects" array in the enclosing "package.json" file

    node_modules/@interactjs/auto-start/package.json:21:2:
      21 │   "sideEffects": [
         ╵   ~~~~~~~~~~~~~

1 warning
(() => {
})();

Versions:

D:\temp>ver

Microsoft Windows [Version 10.0.19044.1889]

D:\temp>node --version
v18.7.0

D:\temp>npm --version
8.15.0

D:\temp>npm list --depth
temp@1.0.0 D:\temp
+-- @interactjs/auto-start@1.10.17
| +-- @interactjs/core@1.10.17
| +-- @interactjs/interact@1.10.17
| `-- @interactjs/utils@1.10.17
`-- esbuild@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY @esbuild/linux-loong64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-android-64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-android-arm64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-darwin-64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-darwin-arm64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-freebsd-64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-freebsd-arm64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-linux-32@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-linux-64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-linux-arm@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-linux-arm64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-linux-mips64le@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-linux-ppc64le@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-linux-riscv64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-linux-s390x@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-netbsd-64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-openbsd-64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-sunos-64@0.15.2
  +-- UNMET OPTIONAL DEPENDENCY esbuild-windows-32@0.15.2
  +-- esbuild-windows-64@0.15.2
  `-- UNMET OPTIONAL DEPENDENCY esbuild-windows-arm64@0.15.2

@evanw
Copy link
Owner

evanw commented Aug 13, 2022

I can confirm that it's a Windows-specific issue. The pattern uses / and paths on Windows use \. I'll have to make esbuild compensate for this.

@evanw evanw closed this as completed in 6fd8736 Aug 14, 2022
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