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

Fix a bug in __require #1604

Closed
wants to merge 1 commit into from
Closed

Fix a bug in __require #1604

wants to merge 1 commit into from

Conversation

lovasoa
Copy link

@lovasoa lovasoa commented Sep 14, 2021

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

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.
@lovasoa
Copy link
Author

lovasoa commented Sep 14, 2021

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
Copy link

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
Copy link

#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
Copy link

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
Copy link

hyrious commented Sep 15, 2021

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
Copy link
Owner

evanw commented Sep 21, 2021

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
Copy link
Owner

evanw commented Sep 21, 2021

Closing this in favor of the Proxy approach.

@evanw evanw closed this Sep 21, 2021
@hyrious
Copy link

hyrious commented Sep 21, 2021

@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.

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

Successfully merging this pull request may close these issues.

Error: Dynamic require of "x" is not supported
4 participants