commonjs: hoists conditionally require()'d ESM side effects

[privatenumber]

• Rollup Plugin Name: @rollup/plugin-commonjs
• Rollup Plugin Version: 22.0.0
• Rollup Version: 2.70.2
• Operating System (or Browser): macOS
• Node Version: v14.16.1
• Link to reproduction (⚠️ read below): https://github.com/privatenumber/issue-reproductions/tree/master/reproductions/rollup/plugins/1177

Input

index.js

if (process.env.NODE_ENV === 'production') {
	require('./prod.js');
}

prod.js

console.log('side effect');

export default 'production';

Output

var src = {};

console.log('side effect');

if (process.env.NODE_ENV === 'production') ;

export { src as default };

Expected Behavior

For the console.log('side effect'); to be delayed until the if-condition passes.

Note, this only happens because prod.js is an ESM file. If the export ... is removed for it to be interpreted as a CJS file, it will compile expectedly:

var src = {};

var prod = {};

var hasRequiredProd;

function requireProd () {
	if (hasRequiredProd) return prod;
	hasRequiredProd = 1;
	console.log('side effect');

	// export default 'production';
	return prod;
}

if (process.env.NODE_ENV === 'production') {
	requireProd();
}

export { src as default };

Actual Behavior

The side effects of the ESM file are hoisted up and executed even if the if-condition doesn't pass.

Additional Information

[lukastaegert]

Yes, conditional requires are only properly supported for CJS targets because it relies on wrapping the target in a function that is later tree-shaken if unused. I currently would not know how to implement that for ESM, we cannot just wrap ES modules in functions.

[lukastaegert]

Please use a dynamic import instead, Rollup core is capable of doing proper tree-shaking for conditional dynamic imports:
https://rollupjs.org/repl/?version=2.70.2&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmxldCUyME5PREVfRU5WJTIwJTNEJTIwJ2RldmVsb3BtZW50JyUzQiU1Q24lNUNuaWYlMjAoTk9ERV9FTlYlMjAlM0QlM0QlM0QlMjAncHJvZHVjdGlvbicpJTIwJTdCJTVDbiU1Q3RpbXBvcnQoJy4lMkZwcm9kLmpzJyklM0IlNUNuJTdEJTIwZWxzZSUyMCU3QiU1Q24lNUN0aW1wb3J0KCcuJTJGZGV2LmpzJyklM0IlNUNuJTVDbiU3RCUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTJDJTdCJTIybmFtZSUyMiUzQSUyMnByb2QuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyY29uc29sZS5sb2coJ3Byb2QnKSUzQiUyMiUyQyUyMmlzRW50cnklMjIlM0FmYWxzZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJkZXYuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyY29uc29sZS5sb2coJ2RldicpJTNCJTIyJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=

[privatenumber]

Thanks for the explanation! Is it possible to emit a warning if its not fully supported?

---

Just curious--in which cases does it not make sense to wrap the module in a function?

[lukastaegert]

Thanks for the explanation! Is it possible to emit a warning if its not fully supported?

PR welcome I guess

Just curious--in which cases does it not make sense to wrap the module in a function?

Whenever you want to only execute it conditionally, or whenever you really need to stick to the strict Node execution order as Node itself works this way.

[stale]

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it. ⓘ