System Format Finaliser does not render all exports for symbol that is exported multiple times

[joeljeske]

• Rollup Version: 2.10.2
• Link to reproduction (IMPORTANT, read below):

https://rollupjs.org/repl/?version=2.10.3&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QiUyMENvbW1vbiUyMCU3RCUyMGZyb20lMjAnLiUyRmNvbW1vbiclM0IlNUNuZXhwb3J0JTIwJTdCJTIwQ29tbW9uJTJDJTIwQ29tbW9uJTIwYXMlMjBPdGhlciUyMCU3RCUzQiU1Q25leHBvcnQlMjBkZWZhdWx0JTIwQ29tbW9uJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIyY29tbW9uLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMGNvbnN0JTIwQ29tbW9uJTIwJTNEJTIwMTIzJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQWZhbHNlJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMnN5c3RlbSUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

Expected Behavior

All exports of a symbol should be rendered if the symbol is exported multiple times:

IN

import { Common } from './common';
export { Common, Common as Other };
export default Common;

EXPECTED

System.register('myBundle', [], function (exports) {
	'use strict';
	return {
		execute: function () {

			const Common = exports('default', 123);
                         exports('Common', Common);
                         exports('Other', Common);
		}
	};
});

Actual Behavior

ACTUAL

System.register('myBundle', [], function (exports) {
	'use strict';
	return {
		execute: function () {

			const Common = exports('default', 123);

		}
	};
});

This works as expected in the other formats, and the plugin hooks are made aware of all of the exports, it seems to be only the system format in which these are missing.

[joeljeske]

So I would have thought that the bug exists somewhere in this code block https://github.com/rollup/rollup/blob/master/src/finalisers/system.ts#L42-L59 but it appears to not be the culprit. Perhaps that exports section is not relevant due to the way that the re-exports work?

I'd be happy to contribute a fix if you could point me the in the right direction.

Thanks!

[guybedford]

@joeljeske chatting with @lukastaegert about this further he suggested that the following output may be more straightforward to implement in this case:

System.register('myBundle', [], function (exports) {
	'use strict';
	return {
		execute: function () {

			const Common = 123;
                        exports({
                          Common: Common,
                          another: Common,
                          default: Common
                        });

		}
	};
});

The problem we have is how to handle live binding exports, since these are emitted within the individual updates (separation of finalizers is not comprehensive!). See for example https://github.com/rollup/rollup/blob/master/src/ast/nodes/AssignmentExpression.ts#L54 - that is, every single AST node that can cause a binding assignment does need to be considered. I don't believe we worry about left hand side nodes in loops though.

It's quite a complex one, but if you'd like to look into it that would be great. @lukastaegert and myself will do our best to advise further if you have any questions.

I also need to see this work myself - so even if you have an incomplete PR, I'd be glad to collaborate on that to ensure we can get this done in the next couple of weeks.

[lukastaegert]

Exactly. Due to their nature, SystemJS exports are all rendered inline at the places where variables are created or updated to ensure the live bindings. So they are spread over AssignmentExpression, ClassDeclaration, ExportDefaultDeclaration, NamespaceVariable, UpdateExpression, VariableDeclaration.

There is one big architectural short-coming at the moment, though, which is that exports are detected by checking if variables have an .exportName set. And at the moment, there is only ever at most one. The best I could think of here is to replace .exportName with an array of .exportNames, but this would touch some more places.

Still if you are willing to investigate, this is very much welcome!

[guybedford]

Note - ideally the const Common = exports('Common', 123) base case should still apply when exportNames.length === 1.

[joeljeske]

I’m definitely very interested, but a bit less confident after how in depth it sounds.. 😬

I don’t quite understand how the live bindings should be handled. What should the rendered output look like with live bindings handled? Do all reexports need to be kept in sync as they originate from a single binding? (In the case of const, live bindings aren’t relevant as opposed to with let and var, right?)

[dasa]

#3246 may be a similar issue

[joeljeske]

Here is an existing test case that should fail given the proper output:
https://github.com/rollup/rollup/blob/master/test/form/samples/no-treeshake/main.js#L4-L5
https://github.com/rollup/rollup/blob/master/test/form/samples/no-treeshake/_expected/system.js#L12

Note that "quux" is not exported from ./main but one would expect it to be.

[joeljeske]

Temporarily, I have patched my install of rollup with this line here https://github.com/rollup/rollup/blob/master/src/Chunk.ts#L1022 to ensure that exports are not removed without errors.

if (exportVariable.exportName && exportVariable.exportName !== exportName && options.format === 'system') {
  throw new Error(`Attempted to bind the same variable to exports ${exportName} and ${exportVariable.exportName} which is not compatible in the current module format. Please assign to a separate variable to support exporting the same value using multiple names.`);
}
exportVariable.exportName = exportName;

My issue was that the rollup plugins are informed that all the exports exist as expected, but they are missing from the bundle itself. This will removes the possibility of green builds that could fail at runtime due to missing exports.

[lukastaegert]

This was closed in #3575