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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

link imports requested by wasm module #1571

Merged
merged 1 commit into from Feb 9, 2017
Merged

Conversation

jayphelps
Copy link
Contributor

@jayphelps jayphelps commented Feb 8, 2017

I wasn't familiar with the systemjs codebase, so it's not entirely clear how the internals work. Is the logic intentionally duplicated between instantiate.js and systemjs-production-loader.js because of file size concerns, etc.

The whole processAnonRegister() thing wasn't clear to me as well. Certainly feel free to suggest changes 馃憤

get_local $value
call $exampleImport
)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this imports example module, linking exampleImport function. It exports a single function exampleExport which when called with an integer will then itself call exampleImport, passing along its return value so we can confirm indeed everything was linked correctly

Copy link
Contributor Author

@jayphelps jayphelps Feb 9, 2017

Choose a reason for hiding this comment

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

For future reader's reference:

This is an "s-expression" textual representation, which was compiled to example.wasm using wabt's wast2wasm, from a build on master at 4492d0cbbb. Both the s-expression and binary are wasm version 0xd. You can probably also use the WebAssembly Explorer (copy/paste into wast section, hit Assemble then Download)

Textual representations of wasm are not yet spec'd so these s-expressions are a convention--what little textual tooling exists so far mostly uses s-expressions. It's extremely likely that a totally different textual format will be standardized. It's mostly used because writing binary by hand is obviously even more impractical.

.then(function (sourceOrModule) {
if (typeof sourceOrModule !== 'string')
return sourceOrModule;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to return, but my code doesn't because I can't figure out what it would return other than undefined?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that's a typo.

execute: function () {
_export(new WebAssembly.Instance(m, importObj).exports);
}
};
Copy link
Contributor Author

@jayphelps jayphelps Feb 8, 2017

Choose a reason for hiding this comment

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

the spec for wasm doesn't currently support linking cyclic dependencies. I'm guessing this whole "setters" thing is to support that for JS modules. Something to keep in mind--it may cause unforeseen problems

Copy link
Member

Choose a reason for hiding this comment

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

The way this will work out for circular cases is just providing an undefined value for exports not yet defined by execution. Interestingly a WASM module importing a JS module in a cycle, will actually support function export hoisting though. Whether that happens in actual engines will be yet to be seen.

@guybedford guybedford merged commit ed8c4b3 into systemjs:master Feb 9, 2017
@guybedford
Copy link
Member

Amazing, thank you so much for picking this up sir!

Yes there is some intentional duplication between the production and development loaders, although shared code is usually managed through common files in most places, and it probably does make sense to make this a shared function between the two at this point.

The processAnonRegister stuff all looks right to me.

Will post out a patch here shortly.

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.

None yet

2 participants