Fix a bug in __require

[lovasoa]

This reverts commit 1dea2db,
which introduced an optimization based on the invalid assumption that the require function will always be either already defined when __require is defined, or never.

Fixes sveltejs/kit#2400

[lovasoa]

This was an "optimization" that was introduced without any comment on the actual performance gains. Since require is in itself an expensive operation, I highly doubt removing an if and a single function call from it leads to any measurable performance gain.

[Conduitry]

For more context, when SvelteKit is producing a production build to run on a Node environment, it uses esbuild for the final bundling of the server-side app. If any of the bundled dependencies from node_modules are written in CJS and require() any Node builtins, esbuild leaves these as require()s in the final bundle, even when the whole app is being bundled to ESM. (sveltejs/kit#1648 (comment))

To get around this, SvelteKit inserts code to define a global require function created using createRequire. This code is, apparently, ending up after esbuild's own runtime code, and so when __require now checks at define time (rather than at call time) whether require is defined, it isn't (yet), and so the global require shim SvelteKit defines is no longer used, and any (untranspiled) requires of Node globals in CJS dependencies of an ESM project end up as function calls that throw a runtime error.

The quickest way back to a working state would indeed probably be to revert this optimization in __require, but in the longer term it would be nice if this shim of SvelteKit's were not necessary and requires of Node builtins in CJS dependencies were transpiled into import statements when bundling to an ESM target. I haven't looked for whether there's already an issue here for that or any related discussion.

[Conduitry]

#946 appears to be the issue to track the underlying thing that resulted in SvelteKit needing this global.require workaround at all. I haven't checked whether the other suggested workarounds in that issue (like using a banner) would help in this case without reverting the change this PR proposes to - i.e., does the banner end up before the __require definition.

[Conduitry]

I should also note that the mechanism SvelteKit is using to define these globals has also since changed since my PR I linked above. As of sveltejs/kit#1822 it is using the official 'inject' feature of esbuild, and it is this that is apparently not defining it in a way or in a place that the __require definition can see it.

[hyrious]

A quick solution would be not generating this require shim when bundling cjs dependencies in esm environment.

Currently this shim just act as a side effect.
The previous version can be safely omitted by rollup/terser because a function is not used.
However current version can not be removed, which causes these errors.

The best solution would be esbuild finding a way to compile cjs code into esm without require, which means converting them to import, like @Conduitry said. However that must be very complicated by just seeing how @rollup/plugin-commonjs does.

[evanw]

This approach won't work. Just reverting the change will re-break #1579 which was trying to use require.resolve. That's the reason why the global require function is currently being preferred if it exists.

I suppose a solution that will work for both edge cases might be a Proxy that forwards property accesses and calls to the global if it exists. That solution seems pretty complicated, however.

[evanw]

Closing this in favor of the Proxy approach.

[hyrious]

@evanw Hi, thank you for looking into this. Just be curious, is it possible to omit this __require when bundling cjs code that not using require in esm environment?

// a.js, should be esm
import "./b"

// b.js, should be cjs
module.exports = 1

// esbuild a.js --bundle
var __require = ... // ← it is not in use, actually
var require_b = __commonJS({ ... })
var import_b = __toModule(require_b());

I don't know the implementation, but I guess a __require is always generated when linking a cjs module in esm env, even though it might not be used.

The very first implementation of __require is just a function, which will be safely removed by rollup/terser/esbuild.
The next 2 impls, actually depend on a runtime toplevel require's value to work, which can not be removed, even though nothing use it. This tiny difference causes esbuild minify to include more useless codes.

---

Another possible solution would be not generating this shim at all, since you can always get an error when accessing a nonexist variable/function.