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

Added AMD externs so that Binaryen builds fine #5748

Merged
merged 1 commit into from
Nov 8, 2017
Merged

Added AMD externs so that Binaryen builds fine #5748

merged 1 commit into from
Nov 8, 2017

Conversation

nazar-pc
Copy link
Contributor

@nazar-pc nazar-pc commented Nov 8, 2017

This is a fix for building Binaryen.js as noted here: #5720 (comment)

With this change I've compiled Binaryen.js master branch without issues.

@dcodeIO, does it work for you too?

@@ -1100,3 +1100,10 @@ var GL;
* @suppress {undefinedVars}
*/
var SDL;

// Module loaders externs, primarily for Binaryen
Copy link
Member

Choose a reason for hiding this comment

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

If this is primarily for binaryen, could this be done there instead?

Copy link
Contributor Author

@nazar-pc nazar-pc Nov 8, 2017

Choose a reason for hiding this comment

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

I believe it is not possible to pass additional arguments like externs to Closure Compiler externally, at least not right now. Also Binaryen is tightly related project.

However, allowing external arguments to Closure Compiler would be useful in general if someone can implement it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I see.

Let's leave this as it is, then. Also I think define is from AMD, so it could help other things too, so makes sense to do it here.

lgtm, just waiting to merge on your ping to @dcodeIO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me to have define being declared. For instance, wabt.js has a very similar wrapper that will benefit from this.

Ideally, binaryen.js would have its own specific externs. That would also remove the necessity to use [' ... '] everywhere to prevent renaming. But that's another story, of course.

@kripken kripken merged commit cee57b6 into emscripten-core:incoming Nov 8, 2017
@nazar-pc nazar-pc deleted the amd-externs branch November 8, 2017 23:20
@dcodeIO
Copy link
Contributor

dcodeIO commented Nov 9, 2017

Confirming that it compiles again.

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.

3 participants