fix #455: "module.require" is an alias for "require"

CHANGELOG.md

@@ -6,6 +6,12 @@
 
     The relative path fix in the previous release caused a regression where paths in source maps contained `\` instead of `/` on Windows. That is incorrect because source map paths are URLs, not file system paths. This release replaces `\` with `/` for consistency on Windows.
 
+* `module.require()` is now an alias for `require()` ([#455](https://github.com/evanw/esbuild/issues/455))
+
+    Some packages such as [apollo-server](https://github.com/apollographql/apollo-server) use `module.require()` instead of `require()` with the intent of bypassing the bundler's `require` and calling the underlying function from `node` instead. Unfortunately that doesn't actually work because CommonJS module semantics means `module` is a variable local to that file's CommonJS closure instead of the host's `module` object.
+
+    This wasn't an issue when using `apollo-server` with Webpack because the literal expression `module.require()` is automatically rewritten to `require()` by Webpack: [webpack/webpack#7750](https://github.com/webpack/webpack/pull/7750). To get this package to work, esbuild now matches Webpack's behavior here. Calls to `module.require()` will become external calls to `require()` as long as the required path has been marked as external.
+
 ## 0.7.15
 
 * Lower `export * as` syntax for ES2019 and below

internal/js_parser/js_parser.go

@@ -7562,7 +7562,7 @@ func (p *parser) jsxStringsToMemberExpression(loc logger.Loc, parts []string, as
 		if i+1 == len(parts) {
 			targetIfLast = assignTarget
 		}
-		if expr, ok := p.maybeRewritePropertyAccess(loc, targetIfLast, js_ast.OptionalChainNone, value, parts[i], loc); ok {
+		if expr, ok := p.maybeRewritePropertyAccess(loc, targetIfLast, js_ast.OptionalChainNone, value, parts[i], loc, false); ok {
 			value = expr
 		} else {
 			value = js_ast.Expr{Loc: loc, Data: &js_ast.EDot{
@@ -7662,6 +7662,7 @@ func (p *parser) maybeRewritePropertyAccess(
 	target js_ast.Expr,
 	name string,
 	nameLoc logger.Loc,
+	isCallTarget bool,
 ) (js_ast.Expr, bool) {
 	if id, ok := target.Data.(*js_ast.EIdentifier); ok {
 		// Rewrite property accesses on explicit namespace imports as an identifier.
@@ -7710,6 +7711,14 @@ func (p *parser) maybeRewritePropertyAccess(
 				p.recordUsage(item.Ref)
 				return p.handleIdentifier(nameLoc, assignTarget, &js_ast.EIdentifier{Ref: item.Ref}), true
 			}
+
+			// Rewrite "module.require()" to "require()" for Webpack compatibility.
+			// See https://github.com/webpack/webpack/pull/7750 for more info.
+			if isCallTarget && id.Ref == p.moduleRef && name == "require" {
+				p.ignoreUsage(p.moduleRef)
+				p.recordUsage(p.requireRef)
+				return js_ast.Expr{Loc: nameLoc, Data: &js_ast.EIdentifier{Ref: p.requireRef}}, true
+			}
 		}
 
 		// If this is a known enum value, inline the value of the enum
@@ -8597,7 +8606,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 		out = exprOut{childContainsOptionalChain: containsOptionalChain}
 		if str, ok := e.Index.Data.(*js_ast.EString); ok {
 			name := js_lexer.UTF16ToString(str.Value)
-			if value, ok := p.maybeRewritePropertyAccess(expr.Loc, in.assignTarget, e.OptionalChain, e.Target, name, e.Index.Loc); ok {
+			if value, ok := p.maybeRewritePropertyAccess(expr.Loc, in.assignTarget, e.OptionalChain, e.Target, name, e.Index.Loc, isCallTarget); ok {
 				return value, out
 			}
 		}
@@ -8794,7 +8803,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 
 		// Potentially rewrite this property access
 		out = exprOut{childContainsOptionalChain: containsOptionalChain}
-		if value, ok := p.maybeRewritePropertyAccess(expr.Loc, in.assignTarget, e.OptionalChain, e.Target, e.Name, e.NameLoc); ok {
+		if value, ok := p.maybeRewritePropertyAccess(expr.Loc, in.assignTarget, e.OptionalChain, e.Target, e.Name, e.NameLoc, isCallTarget); ok {
 			return value, out
 		}
 		return js_ast.Expr{Loc: expr.Loc, Data: e}, out

scripts/end-to-end-tests.js

@@ -111,6 +111,57 @@
     }),
   )
 
+  // Test CommonJS semantics
+  tests.push(
+    // "module.require" should work with internal modules
+    test(['--bundle', 'in.js', '--outfile=out.js', '--format=cjs'], {
+      'in.js': `export {foo, req} from './foo'`,
+      'foo.js': `exports.req = module.require; exports.foo = module.require('./bar')`,
+      'bar.js': `exports.bar = 123`,
+      'node.js': `if (require('./out').foo.bar !== 123 || require('./out').req !== undefined) throw 'fail'`,
+    }),
+    test(['--bundle', 'in.js', '--outfile=out.js', '--format=cjs'], {
+      'in.js': `export {foo, req} from './foo'`,
+      'foo.js': `exports.req = module['require']; exports.foo = module['require']('./bar')`,
+      'bar.js': `exports.bar = 123`,
+      'node.js': `if (require('./out').foo.bar !== 123 || require('./out').req !== undefined) throw 'fail'`,
+    }),
+
+    // "module.require" should work with external modules
+    test(['--bundle', 'in.js', '--outfile=out.js', '--format=cjs', '--external:fs'], {
+      'in.js': `export {foo} from './foo'`,
+      'foo.js': `exports.foo = module.require('fs').exists`,
+      'node.js': `if (require('./out').foo !== require('fs').exists) throw 'fail'`,
+    }),
+    test(['--bundle', 'in.js', '--outfile=out.js', '--format=cjs'], {
+      'in.js': `export {foo} from './foo'`,
+      'foo.js': `let fn = (m, p) => m.require(p); exports.foo = fn(module, 'fs').exists`,
+      'node.js': `try { require('./out') } catch (e) { return } throw 'fail'`,
+    }),
+
+    // "module.exports" should behave like a normal property
+    test(['--bundle', 'in.js', '--outfile=out.js', '--format=cjs'], {
+      'in.js': `export {foo} from './foo'`,
+      'foo.js': `exports.foo = module.exports`,
+      'node.js': `if (require('./out').foo !== require('./out').foo.foo) throw 'fail'`,
+    }),
+    test(['--bundle', 'in.js', '--outfile=out.js', '--format=cjs'], {
+      'in.js': `export {default} from './foo'`,
+      'foo.js': `module.exports = 123`,
+      'node.js': `if (require('./out').default !== 123) throw 'fail'`,
+    }),
+    test(['--bundle', 'in.js', '--outfile=out.js', '--format=cjs'], {
+      'in.js': `export {default} from './foo'`,
+      'foo.js': `let m = module; m.exports = 123`,
+      'node.js': `if (require('./out').default !== 123) throw 'fail'`,
+    }),
+    test(['--bundle', 'in.js', '--outfile=out.js', '--format=cjs'], {
+      'in.js': `export {default} from './foo'`,
+      'foo.js': `let fn = (m, x) => m.exports = x; fn(module, 123)`,
+      'node.js': `if (require('./out').default !== 123) throw 'fail'`,
+    }),
+  )
+
   // Test internal CommonJS export
   tests.push(
     test(['--bundle', 'in.js', '--outfile=node.js'], {