[core] incorrect output of exporting of reference

[JounQin]

• Rollup Version: 2.45.2
• Operating System (or Browser): macOS
• Node Version (if applicable): v14.16.0
• Link to reproduction (IMPORTANT, read below): REPL

Expected Behavior

import { noop } from 'ts-essentials'

export * from 'ts-essentials';
export const badNoop = noop;

Actual Behavior

export * from 'ts-essentials';
export { badNoop } from 'ts-essentials';

[JounQin]

related: angular/angular#40959

[JounQin]

@lukastaegert Hi, is there any news, it's a really serious problem...

[lukastaegert]

I will have a look into this the next days. Sorry there was no news as I was busy on another front.

[lukastaegert]

Ok, this is not as simple as it looks. The code Rollup produces is not correct, but it is not immediately clear how to really hand this correctly. The core of the problem is that in main.js, you first reexport * from './types' and then * from './utils'. If you switch the order of these statements, everything will be fine.

Why is that?

The core of the problem is that ts-essentials is external. So Rollup does not know which exports it contains, and currently it blindly guesses that it contains all possible exports. Therefore when Rollup tries to determine what an export of badNoop will export, it looks at main.js, sees the first star reexport, follows that, finds the star reexport from ts-essentials and thus assumes the export is satisfied directly via ts-essentials. And this is correct IF ts-essentials actually contains an export of that name. In that scenario, your suggested code change would be wrong, because the named export would falsely have precedence over the star reexport.

Note that the reason that badNoop becomes an explicit export is what Rollup could improve here, because that is useless and confusing.

HOWEVER, if there is no export named badNoop, then the bundled code is wrong. Because in that situation, it surely should favour the export via utils, but it does not.

The problem is, I have no clue how to fix this well except by creating an additional chunk, because as exports in ESM are static, we cannot determine them at runtime. And dynamically creating a chunk here is quite difficult as well.

[JounQin]

It seems it worked previously, at least on ng-packagr@10 which was using rollup@2.8. (I'm not on a computer, so I can't ensure if it works on rollup@2.8)

[JounQin]

ESM is static, so of course rollup should know about whether badNoop is available in ts-essentials, right?

And also, I don't know if it's valid to override export * exports with a same identifier?

export * from 'ts-essentials' // it contains `noop`
export const noop = {
  console.log('my nopp')
}

If it is valid, rollup should always skip assuming * contains all exports, but should keep it as is.

If it is invalid, rollup should always emit an error to report instead.

[JounQin]

According to tc39/proposal-export-default-from#11, rollup should not care whether badNopp or even noop has been exported via export * from 'ts-essentials' at all.

[lukastaegert]

Yes, the problem is really the competing namespace reexports in main.js. Nevertheless, there is an argument to be made to favour the latter export: In case there is more than one namespace reexport in a file and both reexport the same name, then neither would take precedence (not sure if an error would be thrown, but something in that direction). So in the interest of the user it is always a good guess that there is no conflict, in which case we should favour the definitely existing export from the second namespace reexport over the only potentially existing on of the first. Will see if I can implement something here.

[lukastaegert]

Fix at #4064

[lukastaegert]

Fixed REPL: https://rollupjs.org/repl/?pr=4064&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMColMjBmcm9tJTIwJy4lMkZ0eXBlcyclNUNuZXhwb3J0JTIwKiUyMGZyb20lMjAnLiUyRnV0aWxzJyU1Q24lMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJ0eXBlcy5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjAqJTIwZnJvbSUyMCU1QyUyMnRzLWVzc2VudGlhbHMlNUMlMjIlM0IlNUNuJTIyJTdEJTJDJTdCJTIybmFtZSUyMiUzQSUyMnV0aWxzLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QiUyMG5vb3AlMjAlN0QlMjBmcm9tJTIwJTVDJTIyLiUyRnR5cGVzJTVDJTIyJTNCJTVDbiU1Q25leHBvcnQlMjBjb25zdCUyMGJhZE5vb3AlMjAlM0QlMjBub29wJTNCJTVDbiUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlcyUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

[JounQin]

@lukastaegert I think it should prefer the later one, instead of locally.

https://rollupjs.org/repl/?pr=4064&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMColMjBmcm9tJTIwJy4lMkZ1dGlscyclNUNuZXhwb3J0JTIwKiUyMGZyb20lMjAnLiUyRnR5cGVzJyU1Q24lMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJ0eXBlcy5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjAqJTIwZnJvbSUyMCU1QyUyMnRzLWVzc2VudGlhbHMlNUMlMjIlM0IlNUNuJTIyJTdEJTJDJTdCJTIybmFtZSUyMiUzQSUyMnV0aWxzLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QiUyMG5vb3AlMjBhcyUyMF9ub29wJTIwJTdEJTIwZnJvbSUyMCU1QyUyMi4lMkZ0eXBlcyU1QyUyMiUzQiU1Q24lNUNuZXhwb3J0JTIwY29uc3QlMjBub29wJTIwJTNEJTIwKCklMjAlM0QlM0UlMjAlN0IlNUNuJTIwJTIwY29uc29sZS5sb2coJ215JTIwbG9vcCcpJTVDbiU3RCUzQiU1Q24lMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

[lukastaegert]

I think it should prefer the later one, instead of locally

Not sure I fully understand. What would you expect to happen in your example?

[JounQin]

I think it should prefer the later one, instead of locally

Not sure I fully understand. What would you expect to happen in your example?

const noop = () => {
  console.log('my loop');
};

export { noop };

export * from 'ts-essentials'; // it contains `noop`, maybe should override above `noop`, not sure

But it should leave it as-is IMO.

[lukastaegert]

Here is a small example that you can run in your browser if you put index.js in a script tag with "type"="module":

// index.js
import * as ns from './reexport.js';
console.log(ns.foo, ns.bar, ns.baz);

// reexport.js
export * from './first.js';
export * from './second.js';

// first.js
export const foo = 1;
export const bar = 1;

// second.js
export const foo = 2;
export const baz = 2;

If you run this, the system will log undefined 1 2. That means the conflicting reexport foo is just removed from the namespace, there is no preference in real runtimes. If you replace index.js with

import { foo, bar, baz } from './reexport.js';
console.log(foo, bar, baz);

then the code will immediately throw Uncaught SyntaxError: The requested module './reexport.js' contains conflicting star exports for name 'foo'.

So no, it should not prefer any one of them.

[lukastaegert]

So the logic that is implemented in the PR is a heuristic that assumes that there will be no conflicts, which means if you explicitly export a name in one of the modules the namespace of which is reexported, then we assume this name does not exist in any of the other namespaces, especially not in external namespaces.

[lukastaegert]

But it will not replicate real browser errors like the one listed above.

[JounQin]

I mean we should keep the exporting order, so that the JavaScript engine will emit the error.

Or, we should provide an option to:

1. keep order, then it will emit error at runtime
2. emit error on building
3. reorder to prefer local one

[lukastaegert]

I do not think it is possible to emit any error at runtime when bundling a single file as the error only exists when there are actually several conflicting namespace reexports from different files, so you need at least two local chunks to simulate it. Beyond that, I do not think this error is really useful to many, so I would prefer to only add such an option if there is actual demand.

[lukastaegert]

Note that explicit named exports or reexports will always override the same names in namespace reexports and will not throw even in case of conflict.

[guybedford]

#4049 (comment)

This actually seems to me to be an implementation bug as ResolveExport per https://tc39.es/ecma262/#sec-resolveexport is still called on namespace exotic object Get access, which should then throw the ambiguous error. So it seems this is a bug in Firefox AND Chrome. If someone can please test Safari as well that would be a great help. Will verify further and post browser bugs as appropriate.

[guybedford]

Posted tc39/ecma262#2399 it turns out this is a spec thing, and will likely be just an editorial change to codify the behaviour at this point as a dynamic error is likely not web compatible.