[mbostock]

This adds sideEffects: false to our package.json under the rationale that the side effect in question here (the assignment to Mark.prototype.plot to avoid the circular dependency) is entirely internal to Plot, unlike a side effect such as injecting a stylesheet or a global prototype polyfill. The intent of this is to allow esm.sh to bundle Plot more efficiently.

[Fil]

This test/bundle-test.js works against main and fails in the PR:

import commonjs from "@rollup/plugin-commonjs";
import {nodeResolve} from "@rollup/plugin-node-resolve";
import {rollup} from "rollup";
import {assert, it} from "vitest";

it("bundled marks have a plot method", async () => {
  const entry = {
    resolveId: (id) => (id === "entry" ? id : null),
    load: (id) => (id === "entry" ? `import {dot} from "./src/index.js"; export default dot().plot;` : null)
  };
  const bundle = await rollup({input: "entry", plugins: [entry, commonjs(), nodeResolve()]});
  const {output} = await bundle.generate({format: "iife", name: "result"});
  assert.strictEqual(new Function(`${output[0].code}; return typeof result;`)(), "function");
});

ref. rollup/rollup#5538 (comment) for a solution that would work for rollup (but maybe not for other bundlers?).

[mbostock]

Ugh, stupid bundlers. Thanks for finding.

[mbostock]

Hmmm… I can’t reproduce the bug in the Rollup REPL.

https://rollupjs.org/repl/?version=4.59.0&shareable=eyJleGFtcGxlIjpudWxsLCJtb2R1bGVzIjpbeyJjb2RlIjoiaW1wb3J0IHtNYXJrfSBmcm9tIFwiLi9tYXJrLmpzXCI7XG5pbXBvcnQge3Bsb3R9IGZyb20gXCIuL3Bsb3QuanNcIjtcblxuLy8gTm90ZTogdGhpcyBzaWRlIGVmZmVjdCBhdm9pZHMgYSBjaXJjdWxhciBkZXBlbmRlbmN5LlxuTWFyay5wcm90b3R5cGUucGxvdCA9IGZ1bmN0aW9uICh7bWFya3MgPSBbXSwgLi4ub3B0aW9uc30gPSB7fSkge1xuICByZXR1cm4gcGxvdCh7Li4ub3B0aW9ucywgbWFya3M6IFsuLi5tYXJrcywgdGhpc119KTtcbn07XG5cbmV4cG9ydCB7cGxvdH0gZnJvbSBcIi4vcGxvdC5qc1wiO1xuZXhwb3J0IHtNYXJrfSBmcm9tIFwiLi9tYXJrLmpzXCI7XG5leHBvcnQge0RvdH0gZnJvbSBcIi4vZG90LmpzXCI7IiwiaXNFbnRyeSI6dHJ1ZSwibmFtZSI6Im1haW4uanMifSx7ImNvZGUiOiJpbXBvcnQge01hcmt9IGZyb20gXCIuL21hcmsuanNcIjtcblxuZXhwb3J0IGZ1bmN0aW9uIHBsb3Qoe21hcmtzID0gW119ID0ge30pIHtcbiAgcmV0dXJuIGBwbG90KCR7bWFya3N9KWA7XG59IiwiaXNFbnRyeSI6ZmFsc2UsIm5hbWUiOiJwbG90LmpzIn0seyJjb2RlIjoiZXhwb3J0IGNsYXNzIE1hcmsge1xuICB0b1N0cmluZygpIHtcbiAgICByZXR1cm4gXCJNYXJrXCI7XG4gIH1cbn0iLCJpc0VudHJ5IjpmYWxzZSwibmFtZSI6Im1hcmsuanMifSx7ImNvZGUiOiJpbXBvcnQge01hcmt9IGZyb20gXCIuL21hcmsuanNcIjtcblxuZXhwb3J0IGNsYXNzIERvdCBleHRlbmRzIE1hcmsge1xuICB0b1N0cmluZygpIHtcbiAgICByZXR1cm4gXCJEb3RcIjtcbiAgfVxufSIsImlzRW50cnkiOmZhbHNlLCJuYW1lIjoiZG90LmpzIn1dLCJvcHRpb25zIjp7Im91dHB1dCI6eyJmb3JtYXQiOiJlcyJ9LCJ0cmVlc2hha2UiOnsibW9kdWxlU2lkZUVmZmVjdHMiOmZhbHNlfX19

[mbostock]

So somewhat astonishingly, the behavior depends on whether we add @rollup/plugin-node-resolve…

[mbostock]

I guess that’s because it only checks for sideEffects: false if you have that plugin enabled.

[mbostock]

Your test still doesn’t work because it only imports the dot mark, but not Mark, and hence Rollup thinks that the Mark.prototype.plot assignment is tree-shakable. But I think that’s valid. So I won’t add a test, but I’m going to publish this as a prerelease so we can see what esm.sh does.

[mbostock]

I think I’ve fixed it, but now I’m blocked by esm-dev/esm.sh#1490; esm.sh isn’t currently serving new packages due to being out of disk space.

[mbostock]

esm.sh is back, and I’ve confirmed that this technique now works. It does mean that importing a subclass of Mark by itself (such as dot) won’t include the plot method, though. I’m not sure if that’s fixable… or acceptable. But I don’t think I should overindex on the tree shaking behavior of bundlers; more important that the library loads quickly in the common case.