Skip to content
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

syntheticNamedExport export is appears in renderedExports, even it's not rendered actually #3857

Open
Amareis opened this issue Nov 5, 2020 · 9 comments

Comments

@Amareis
Copy link
Contributor

Amareis commented Nov 5, 2020

Expected Behavior

Synthetic export should not be dropped until it's unused and treeshaking is on.

Actual Behavior

Even with disabled treeshaking and minifyInternalExports, export is dropped. It can break the entry signature, as in example. Also, SNE export is appears in OutputChunk.modules.renderedExports, but it doesn't rendered actually.

As I see, it's happened here: https://github.com/rollup/rollup/blob/master/src/Module.ts#L396
I commented that line and all is fine - even tests is not broken.

It's not treeshaked, even if not used. In commonjs module, where __moduleExports it's just reexported default, rollup uses this default, but don't remove __moduleExports, but as I see it's just current rollup behaviour: don't drop export statement, if it's reexport of included variable, even if export itself is not used.

@Amareis
Copy link
Contributor Author

Amareis commented Nov 5, 2020

D'oh, there is documentation
image
Ok, so it's just about renderedExports.

@Amareis Amareis changed the title syntheticNamedExport export is always dropped from chunk with preserveModules syntheticNamedExport export is appears in renderedExports, even it's not rendered actually Nov 5, 2020
@lukastaegert
Copy link
Member

Exactly. It is only exposed if used. The reason is that this is a central feature for the commonjs plugin, and we do not want to pollute entry points with the artificial __moduleExports export unless absolutely necessary. If renderedExports is inconsistent with the actually rendered exports, then that should be considered a bug. PR welcome if you want to give it a shot!

@Amareis
Copy link
Contributor Author

Amareis commented Nov 5, 2020

Ok, so I think it can be fixed by adding additional field with ectual SNE export name to OutputChunk.modules, so if someone wants to know it, it can be done.

But what with reexport-all from modules with SNE? It breaks entry signatures too and even not warned by rollup. Even without entry signatures it can easly break code when one module reexports all from SNE-module and then third module trying to export some (valid) name from that reexport.

@Amareis
Copy link
Contributor Author

Amareis commented Nov 5, 2020

We can either just forbid to reexport-all from a SNE-module, or "pollute" SNE in that case (and throws if importer module already have SNE, or even merge them). But at least warning is needed.

@lukastaegert
Copy link
Member

Even without entry signatures it can easly break code when one module reexports all from SNE-module and then third module trying to export some (valid) name from that reexport.

Please give this a shot, Rollup should handle that nicely for internal imports. It is only if an entry module is reexporting the namespace of a module with synthetic exports. In that case, the synthetic exports are completely ignored. But in my eyes, this is completely consistent with the synthetic export being omitted if the module itself is the entry point.

@Amareis
Copy link
Contributor Author

Amareis commented Nov 5, 2020

Not only entry modules, but any module. Just look at https://repl.it/repls/RemarkableSlipperyRam#input.js
There is no any warning, and if instead of 1.js and q there will be react and createElement it will pretty confusing.

@lukastaegert
Copy link
Member

Ah I see. So you are importing from a module without synthetic exports that reexports from a module that has synthetic exports. I am sure this case was not considered. Looks like a bug to be fixed.

@Amareis
Copy link
Contributor Author

Amareis commented Dec 1, 2020

Ok, so I returned to this issue. There is second case, which is described in top post, but not discussed. https://repl.it/@Amareis/RemarkableSlipperyRam

const t = {q: 1}
export {t as __moduleExports} //__moduleExports is marked as synthetic export

This file is not entry. After translating with preserveModules, __moduleExports is dropped from chunk code and t becomes exported. I don't think it should be removed, especially if minifyInternalExports is disabled.

@lukastaegert
Copy link
Member

#3894 contains a fix for the bug concerning namespace reexports you mentioned earlier, please have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants