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

add a .then() method to the module-creating function in MODULARIZE #5085

Merged
merged 4 commits into from
Apr 3, 2017

Conversation

kripken
Copy link
Member

@kripken kripken commented Mar 27, 2017

This lets it be used as a promise, and works in both sync and async mode (we need async for wasm).

In modularize mode, the entire output is a function Module() (or another name, set by EXPORT_NAME). This is useful for libraries, so they don't influence the global scope. Til now most libraries, like box2d.js and ammo.js, assumed synchronous startup, which was made possible by not having a mem init file (which was convenient anyhow as for libraries having extra files is less good). But for wasm it looks like we can't depend on sync compilation.

This PR adds a then() method, usable as

Module().then(function(Module) { .. });

Basically like a promise. The callback is called when the module is all compiled and ready to be used. It has the module as a parameter.

This is also perhaps a nicer API than the other methods for finding out when it is possible to call compiled code (e.g., onRuntimeInitialized), so in the long term, maybe we'd want to recommend people use modularize more, and this with it.

The wasm branch of ammo.js uses this. Looks like it works ok.

…llowing it to be used like a promise, and providing a uniform API for both sync and async compilation
@bvibber
Copy link
Collaborator

bvibber commented Mar 27, 2017

Looks good to me!

I ran into same problem when adding wasm mode to ogv.js; this looks like a handier solution than the manual use of onRuntimeInitialized I initially used, though I'll have to retool some of my initialization code away from the 'init function looks like a constructor' pattern to 'init function looks like a Promise' pattern.

@bvibber
Copy link
Collaborator

bvibber commented Mar 27, 2017

(Would it make sense to also have a .catch() that calls if we never initialize due to an error?)

@kripken
Copy link
Member Author

kripken commented Mar 27, 2017

catch is a little trickier. We'd need to add a try-catch in a bunch of places - basically anywhere that can throw during startup, and their async callbacks. Or we could use window.error perhaps, but that could have weird interactions if the rest of the page uses it?

If all the rest of our startup were Promises that had catches, this might be simple, and maybe that's worth working towards eventually.

@kripken
Copy link
Member Author

kripken commented Mar 30, 2017

Tested also on box2d.js, in the wasm branch there. Looks good there too.

src/postamble.js Outdated
func(Module);
});
if (old) old();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't old be called before func ?
I mean, in Module().then(f1).then(f2), I want f1 to always be called before f2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, I reordered it, thanks.

@drola
Copy link
Contributor

drola commented Apr 2, 2017

Working towards a .catch() is preferable because:

  1. then() hints at usage of promises and error handling using promise rejection is consistent with that pattern.
  2. Encapsulating all possible error conditions and exposing them as a promise rejection will simplify usage of modules
  3. It's a general principle to avoid using globals as much as possible. We should avoid using window.error. These modules will eventually be packed together with other frameworks and packaging tools such as Webpack or dependency injection frameworks.

@kripken
Copy link
Member Author

kripken commented Apr 3, 2017

Agreed, yeah, we should work towards a catch as a followup. (Perhaps we can refactor all the code that runs after it into a scope and use a normal Promise, or something like that? I don't know offhand if Promises are that easy to build on.)

Merging this for now as it's an improvement in the right direction, and already used by ammo.js and box2d.js successfully.

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