[mbostock]

This distributes Plot exclusively as a bundle, rather than shipping the unbundled source files. This in turn allows sideEffects: false, and allows us to resume using an (internal) circular dependency to more cleanly describe the relationship between Mark and the plot function. This approach further allows 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.

[mbostock]

My plan now is to add a Rollup ES module bundle to this library, and ship that, so that we are not dependent on others’ bundlers for correct behavior. I can then go back to using circular imports (which are perfectly valid according to the specification), but those circular imports won’t be visible outside this library because we’ll only publish a single module.