Skip to content

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Mar 19, 2024

I believe the "ready" promise was set on the Module object here primarily so that closure would not minify the name. Instead of polluting the Module namespace we can instead just tell closure not to minify it.

In addition to polluting the Module namespace exporting a ready promise is also confusing since the only way to get a module object when -sMODULARIZE is used is via waiting on the promise returned from the factory function. So, by definition, the promise has already resolved before anyone can access it.

Instead we use the closure-externs.js file to suppress the minifiction which matches the behavior of the incoming moduleArg argument.

I believe the "ready" promise was set on the Module object here
primarily so that closure would not minify the name.  Instead of
polluting the Module namespace we can instead just tell closure not to
minify it.

In addition to polluting the `Module` namespace exporting a `ready`
promise is also confusing since the only way to get a module object when
`-sMODULARIZE` is used is via waiting on the promise returned from the
factory function.  So, by definition, the promise has already resolved
before anyone can access it.

Instead we use the `closure-externs.js` file to suppress the minifiction
which matches the behavior of the incoming `moduleArg` argument.
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if there is not another use for .ready in ES6 modules or CommonJS or something else, but lgtm if @brendandahl confirms no problem there.

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 19, 2024

I am not sure if there is not another use for .ready in ES6 modules or CommonJS or something else, but lgtm if @brendandahl confirms no problem there.

I'm pretty sure there would be no way to call access Module.ready unless you had already resolve the promise returned by the module factory function.

@brendandahl
Copy link
Collaborator

You could technically get access to ready before the promise is resolved since the you can pass in an argument to the ModuleFactory that will then set ready on the argument. e.g.

let m = require('./a.out.js');
let settings = {};
m(settings);
console.log(settings.ready);
// main wouldn't run until next tick

Anyway, I don't really see a need for defining it on the module and returning it from the ModuleFactory for modularize/es6.

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 19, 2024

You could technically get access to ready before the promise is resolved since the you can pass in an argument to the ModuleFactory that will then set ready on the argument. e.g.

let m = require('./a.out.js');
let settings = {};
m(settings);
console.log(settings.ready);
// main wouldn't run until next tick

Very tricky yes. I wonder if we should remove that feature? i.e. should we moduleArg as purely readonly and then Object.assign(Module, moduleArg)

Anyway, I don't really see a need for defining it on the module and returning it from the ModuleFactory for modularize/es6.

Agreed. I think this should be fine to land.

@sbc100 sbc100 merged commit 5dbbd71 into emscripten-core:main Mar 19, 2024
@sbc100 sbc100 deleted the read_promise branch March 19, 2024 23:39
@curiousdannii
Copy link
Contributor

This probably warrants a mention in the changelog.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 20, 2024
sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 20, 2024
sbc100 added a commit that referenced this pull request Mar 20, 2024
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.

4 participants