Circular dependency issue

[arcanis]

I'm porting the build system for Yarn and it seems we hit a pathological case where ESBuild incorrectly resolves cyclic dependencies. To reproduce:

git clone git@github.com:yarnpkg/berry
git checkout 9f7809c2b5f620203626b8f9c9f47e451863f5f5
yarn build:cli --no-minify
YARN_IGNORE_PATH=1 node ./packages/yarnpkg-cli/bundles/yarn.js --version

The problem seems to be around the two following lines:

https://github.com/yarnpkg/berry/blob/17beb526719627a677585f5ce0b9ced146a58def/packages/plugin-pnp/sources/PnpLinker.ts#L8
https://github.com/yarnpkg/berry/blob/17beb526719627a677585f5ce0b9ced146a58def/packages/plugin-pnp/sources/index.ts#L6

It causes PnpLinker to be undefined in index.ts, thus crashing the rest of the program since it then gets stored here as undefined as well. Since the codebase only ever access PnpLinker.ts via index.ts, I don't see why it would be undefined.

[evanw]

Just got a chance to look at this. It looks like this is because of the use of require() in one spot: getPluginConfiguration.ts. Here's a change that gets it to work:

diff --git a/packages/yarnpkg-cli/sources/tools/getPluginConfiguration.val.js b/packages/yarnpkg-cli/sources/tools/getPluginConfiguration.val.js
index 440cbbf6..f1e3d5c3 100644
--- a/packages/yarnpkg-cli/sources/tools/getPluginConfiguration.val.js
+++ b/packages/yarnpkg-cli/sources/tools/getPluginConfiguration.val.js
@@ -2,8 +2,9 @@
 // return value is what's used within the bundle (cf val-loader).
 
 module.exports = ({modules, plugins}) => {
-  return {code: `exports.getPluginConfiguration = () => ({\n  modules: new Map([\n${modules.map(request =>
-    `    [${JSON.stringify(require(`${request}/package.json`).name)}, require(${JSON.stringify(request)})],\n`
+  return {code: `${modules.map((request, i) => `import _${i} from ${JSON.stringify(request)}\n`).join(`\n`)}
+export const getPluginConfiguration = () => ({\n  modules: new Map([\n${modules.map((request, i) =>
+    `    [${JSON.stringify(require(`${request}/package.json`).name)}, _${i}],\n`
   ).join(``)}\n  ]),\n  plugins: new Set([\n${plugins.map(request =>
     `    ${JSON.stringify(require(`${request}/package.json`).name)},\n`
   ).join(``)}\n  ]),\n});\n`};

It just changes CommonJS to ESM for this generated file. I'm not totally sure what the semantics are of these files so I'm not sure if it's equivalent to use the default export for these, but the CJS-to-ESM conversion is the relevant part.

[evanw]

Specifically, when an ESM file is the target of require(), esbuild wraps it up in a closure so it can be lazily-evaluated. Here's a simplified way to reproduce this:

// plugin-pnp/PnpLinker.js
import { getPnpPath } from './index'
export class PnpLinker { path = getPnpPath() }

// plugin-pnp/index.js
import { PnpLinker } from './PnpLinker'
export const getPnpPath = () => ({})
const plugin = { linker: new PnpLinker }
export default plugin

// entry.js
require('./plugin-pnp/index')

When you bundle entry.js, esbuild will generate this (omitting the wrapper code):

// plugin-pnp/index.js
var require_plugin_pnp = __commonJS((exports) => {
  __export(exports, {
    default: () => plugin_pnp_default,
    getPnpPath: () => getPnpPath2
  });
  var getPnpPath2 = () => ({});
  var plugin = {linker: new PnpLinker()};
  var plugin_pnp_default = plugin;
});

// plugin-pnp/PnpLinker.js
var import_index = __toModule(require_plugin_pnp());
var PnpLinker = class {
  path = import_index.getPnpPath();
};

// entry.js
require_plugin_pnp();

I suppose this problem happens because esbuild is only adding the CommonJS wrapper to the required file and not to all of its transitive dependencies. Maybe I should change esbuild to do that instead. But you wouldn't want esbuild to do that in this case anyway because that would have negative implications for code size and would prevent tree shaking. Avoiding using require() on your ESM code seems like what you'd want to be doing regardless of esbuild's behavior here.

[ggoodman]

I've had some success getting around this behaviour by using

import Module from 'module';

const appRequire = Module.createRequire(import.meta.url);

Perhaps this could help?

[evanw]

This should be fixed in version 0.11.0 because esbuild now recursively wraps all transitive dependencies of a require(). The generated code for the example above looks something like this now:

// plugin-pnp/PnpLinker.js
var PnpLinker;
var init_PnpLinker = __esm(() => {
  init_plugin_pnp();
  PnpLinker = class {
    path = getPnpPath();
  };
});

// plugin-pnp/index.js
var plugin_pnp_exports = {};
__export(plugin_pnp_exports, {
  default: () => plugin_pnp_default,
  getPnpPath: () => getPnpPath
});
var getPnpPath, plugin, plugin_pnp_default;
var init_plugin_pnp = __esm(() => {
  init_PnpLinker();
  getPnpPath = () => ({});
  plugin = {linker: new PnpLinker()};
  plugin_pnp_default = plugin;
});

// entry.js
init_plugin_pnp();