rollup with --preserve-symlinks erroneously assumes all configs are commonjs

[dgoldstein0]

• Rollup Version: 2.27.1-2.38.4
• Operating System (or Browser): Ubuntu 16.04
• Node Version (if applicable): 14.1.0
• Link to reproduction: https://repl.it/@dgoldstein0/rollup-repro

the important elements here:

• use rollup 2.27.1 or newer (bug was introduced in 2.27.1)
• reference the config via a symlink
• use --preserve-symlinks when invoking node

Expected Behavior

rollup should parse the config as ESM, and correctly build the bundle after that.

Actual Behavior

Syntax error when parsing the config file on "export".

[dgoldstein0]

bug was introduced in #3783 given that's the only difference between 2.27.0 which works and 2.27.1 which has this bug

[kzc]

If "preserve symlinks" is enabled in the node process, it should be imposed on Rollup.

Here's a fix that passes npm test and NODE_PRESERVE_SYMLINKS=1 npm test. Two tests used to fail in the latter scenario for an unpatched rollup v2.39.0 build. One test now works, and the other one is disabled in the patch if the node process running mocha has "preserve symlinks" enabled.

diff --git a/cli/run/loadConfigFile.ts b/cli/run/loadConfigFile.ts
index 4aa7ac1..f05b690 100644
--- a/cli/run/loadConfigFile.ts
+++ b/cli/run/loadConfigFile.ts
@@ -1,10 +1,10 @@
 import { bold } from 'colorette';
-import * as fs from 'fs';
 import * as path from 'path';
 import { pathToFileURL } from 'url';
 import * as rollup from '../../src/node-entry';
 import { MergedRollupOptions } from '../../src/rollup/types';
 import { error } from '../../src/utils/error';
+import { realpathSync } from '../../src/utils/fs';
 import { mergeOptions } from '../../src/utils/options/mergeOptions';
 import { GenericConfigObject } from '../../src/utils/options/options';
 import relativeId from '../../src/utils/relativeId';
@@ -83,7 +83,7 @@ async function getDefaultFromTranspiledConfigFile(
 }
 
 async function loadConfigFromBundledFile(fileName: string, bundledCode: string) {
-	const resolvedFileName = fs.realpathSync(fileName);
+	const resolvedFileName = realpathSync(fileName);
 	const extension = path.extname(resolvedFileName);
 	const defaultLoader = require.extensions[extension];
 	require.extensions[extension] = (module: NodeModule, requiredFileName: string) => {
diff --git a/src/utils/fs.ts b/src/utils/fs.ts
index 4d58925..6754ae2 100644
--- a/src/utils/fs.ts
+++ b/src/utils/fs.ts
@@ -24,6 +24,13 @@ function mkdirpath(path: string) {
 	}
 }
 
+export const nodePreserveSymlinks = !!+(process.env.NODE_PRESERVE_SYMLINKS || 0) ||
+	process.execArgv.indexOf('--preserve-symlinks') >= 0;
+
+export function realpathSync(path: string) {
+	return nodePreserveSymlinks ? path : fs.realpathSync(path);
+}
+
 export function writeFile(dest: string, data: string | Uint8Array) {
 	return new Promise<void>((fulfil, reject) => {
 		mkdirpath(dest);
diff --git a/src/utils/resolveId.ts b/src/utils/resolveId.ts
index 5dc6566..84f0b52 100644
--- a/src/utils/resolveId.ts
+++ b/src/utils/resolveId.ts
@@ -1,5 +1,5 @@
 import { CustomPluginOptions } from '../rollup/types';
-import { lstatSync, readdirSync, realpathSync } from './fs';
+import { lstatSync, nodePreserveSymlinks, readdirSync, realpathSync } from './fs';
 import { basename, dirname, isAbsolute, resolve } from './path';
 import { PluginDriver } from './PluginDriver';
 
@@ -43,6 +43,7 @@ function addJsExtensionIfNecessary(file: string, preserveSymlinks: boolean) {
 }
 
 function findFile(file: string, preserveSymlinks: boolean): string | undefined {
+	preserveSymlinks = preserveSymlinks || nodePreserveSymlinks;
 	try {
 		const stats = lstatSync(file);
 		if (!preserveSymlinks && stats.isSymbolicLink())
diff --git a/test/function/samples/symlink/_config.js b/test/function/samples/symlink/_config.js
index 561b738..ef84531 100644
--- a/test/function/samples/symlink/_config.js
+++ b/test/function/samples/symlink/_config.js
@@ -1,4 +1,6 @@
 module.exports = {
-	skip: process.platform === 'win32',
+	skip: process.platform === 'win32'
+		|| +(process.env.NODE_PRESERVE_SYMLINKS || 0)
+		|| process.execArgv.indexOf('--preserve-symlinks') >= 0,
 	description: 'follows symlinks'
 };

The top post test case without the fix:

$ NODE_PRESERVE_SYMLINKS=0 rollup -c rollup.config_symlink.js --silent
console.log('main entry');

function square(x) {return x*x}

export { square };

$ NODE_PRESERVE_SYMLINKS=1 rollup -c rollup.config_symlink.js --silent
[!] SyntaxError: Unexpected token 'export'
rollup.config_symlink.js:1
export default {
^^^^^^: Unexpected token 'export'

$ node --preserve-symlinks `which rollup` -c rollup.config_symlink.js --silent
[!] SyntaxError: Unexpected token 'export'
rollup.config_symlink.js:1
export default {
^^^^^^: Unexpected token 'export'

The top post test case with the fix:

$ NODE_PRESERVE_SYMLINKS=0 rollup -c rollup.config_symlink.js --silent
console.log('main entry');

function square(x) {return x*x}

export { square };

$ NODE_PRESERVE_SYMLINKS=1 rollup -c rollup.config_symlink.js --silent
console.log('main entry');

function square(x) {return x*x}

export { square };

$ node --preserve-symlinks `which rollup` -c rollup.config_symlink.js --silent
console.log('main entry');

function square(x) {return x*x}

export { square };

It will be a pain to create tests to get 100% code coverage for the patch.

[egonelbre]

Note https://github.com/rollup/plugins/blob/6eb661692f5b8d8fc4e3b61ff748f49fab1e82ec/packages/node-resolve/src/fs.js#L20 probably needs the fix as well.

Since node-resolve is called as a plugin and all the calls check preserveSymlinks bool, it doesn't need it. Although, it might make sense to pass preserveSymlinks as an argument to avoid a chance that someone forgets to use the argument.

[kzc]

I now see that Windows drive handling as seen in #4260 is unrelated to this issue.

However, rollup core already deals with preserveSymlinks , and I still think the patch above is a reasonable fix for #3949 since rollup core needs to know about the state of the preserve symlinks characteristics of the NodeJS process (env var or CLI flag) that is running it and act accordingly.

[egonelbre]

Sure, the CLI part; however I don't think it should override the command-line flag when explicitly specified. Seems weird to rely on env flags inside utils/fs.ts and utils/resolveId.ts; but I don't know too much how rollup is meant to be structured.

[kzc]

If the NodeJS process running rollup is not following symlinks, rollup ought to follow suit as such resolve calls would not work in the traditional NodeJS manner anyway - as evidenced by the bug seen in this issue, and the fact that rollup's test suite fails when this NodeJS mode is active. The only sensible thing to do is either to ignore the rollup flag (perhaps with a warning) or to have rollup halt with an error explaining the conflict.

Side note: the naming of --preserve-symlinks in NodeJS was unfortunate, as the behavior is opposite to its name. The flag really means "ignore symlinks" in path resolution. When used it's actually preserving the original path name - irrespective of the symlink.

[egonelbre]

If the NodeJS process running rollup is not following symlinks, rollup ought to follow suit as such resolve calls would not work in the traditional NodeJS manner anyway.

Sure, I agree with the the change for rollup cli to respect NODE_PRESERVE_SYMLINKS, but only when --preserve-symlinks=true or --preserve-symlinks=false hasn't been explictly passed as an argument. Usually the flag has precedence over the environment variable.

[kzc]

Usually the flag has precedence over the environment variable

Yes that's desirable but the environment variable in question, NODE_PRESERVE_SYMLINKS, is under the control of NodeJS. If the NodeJS process running rollup has NODE_PRESERVE_SYMLINKS=1 in effect it is not obvious from the documentation whether clearing or setting process.env.NODE_PRESERVE_SYMLINKS within rollup to another value mid-process would change NodeJS' behavior without examining the NodeJS implementation. If NodeJS caches that value at process startup it would not work.

And if the NodeJS process running rollup has --preserve-symlinks specified, then as far as I know it's not possible for rollup to override that behavior with --no-preserveSymlinks without completely reimplementing the NodeJS module path resolution algorithm. That could be done of course - esbuild does this very thing in Go - but it would require a great deal of effort.