-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
System Format Finaliser does not render all exports for symbol that is exported multiple times #3566
Comments
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! |
@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. |
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 There is one big architectural short-coming at the moment, though, which is that exports are detected by checking if variables have an Still if you are willing to investigate, this is very much welcome! |
Note - ideally the |
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?) |
#3246 may be a similar issue |
Here is an existing test case that should fail given the proper output: Note that "quux" is not exported from |
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. |
This was closed in #3575 |
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
EXPECTED
Actual Behavior
ACTUAL
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.
The text was updated successfully, but these errors were encountered: