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

Include CJS / AMD module exports under MINIMAL_RUNTIME #23282

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 2, 2025

I don't see why -sMINIMAL_RUNTIME users would not want the same exporting of the module. The EXPORT_ES6 behaviour was already matching. This change is necessary are part of #23261 which feeds the modularized code through closure and our acorn optimizations. Without this closer things the entire module factory is seen as unused and is deleted.

@sbc100 sbc100 requested review from brendandahl and kripken January 2, 2025 22:03
@kripken
Copy link
Member

kripken commented Jan 3, 2025

I would guess this was to save code size. Do we have a setting to know when these exports are needed? If so we could enable it only then. AIUI the general idea of minimal runtime is to add such extra code on an opt-in basis, so users don't get things they aren't actively using.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 3, 2025

I would guess this was to save code size. Do we have a setting to know when these exports are needed? If so we could enable it only then. AIUI the general idea of minimal runtime is to add such extra code on an opt-in basis, so users don't get things they aren't actively using.

The problem is that without this then in #23261 closure compile will see the whole factory function as dead and delete it. So having an option not to include this will break --closure.

However, I'm also noticing that closure isn't working very well in other ways when -sMODULARIZE is enabled. Could we possible say that closure and -sMODULARIZE don't work together? i.e. could we assume that folks using modules will do their own minification/bundling?

@kripken
Copy link
Member

kripken commented Jan 3, 2025

The problem is that without this then in #23261 closure compile will see the whole factory function as dead and delete it. So having an option not to include this will break --closure.

In that case, perhaps adding this in minimal runtime only when closure is enabled makes sense?

But otoh the code size regressions measured here are with closure IIRC, so that wouldn't directly help.

Could we possible say that closure and -sMODULARIZE don't work together? i.e. could we assume that folks using modules will do their own minification/bundling?

Interesting point. Maybe there is some correlation there. But I think that would be a pretty big change.

As background you say that #23261 makes these changes necessary, but I don't quite see the need for that PR - it looks like an internal cleanup, mostly? Is the await change there worth big changes to how we optimize with closure?

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 3, 2025

The problem is that without this then in #23261 closure compile will see the whole factory function as dead and delete it. So having an option not to include this will break --closure.

In that case, perhaps adding this in minimal runtime only when closure is enabled makes sense?

But otoh the code size regressions measured here are with closure IIRC, so that wouldn't directly help.

Could we possible say that closure and -sMODULARIZE don't work together? i.e. could we assume that folks using modules will do their own minification/bundling?

Interesting point. Maybe there is some correlation there. But I think that would be a pretty big change.

As background you say that #23261 makes these changes necessary, but I don't quite see the need for that PR - it looks like an internal cleanup, mostly? Is the await change there worth big changes to how we optimize with closure?

Its not just the await thing. @brendandahl and myself have been bugged by the way modularization happens only after closure for a while now. Doing in the normal JS compiler flow makes quite a few things simpler I think.

I'll see if I can find a way to avoid this.

I don't see why `-sMINIMAL_RUNTIME` users would not want the same
exporting of the module.  The EXPORT_ES6 behaviour was already matching.
This change is necessary are part of emscripten-core#23261 which feeds the modularized
code through closure and our acorn optimizations.  Without this closer
things the entire module factory is seen as unused and is deleted.
@sbc100 sbc100 force-pushed the minimal_modularize branch from 1b2c0ad to 03b9a0d Compare January 3, 2025 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants