Skip to content

Conversation

@dcodeIO
Copy link
Contributor

@dcodeIO dcodeIO commented Sep 22, 2022

Instructs Closure about the types in play so the warnings go away. Didn't encounter the externs warning regarding the WebAssembly object (need to see what CI states). Generally it looks that we could refactor the code slightly to use classes, then we'd not need to annotate in comments so much (except the dynamically generated expression methods).

Related #5062

Update: Seen the WebAssembly object externs warning now, which might require a fix to the externs provided by Emscripten.

@dcodeIO dcodeIO changed the title Fix Closure warnings in Emscripten builds Fix some Closure warnings in Emscripten builds Sep 22, 2022
@dcodeIO
Copy link
Contributor Author

dcodeIO commented Sep 22, 2022

Seems the temporary require fix does what is necessary. With it, the PR on the Emscripten end would not need to be reverted for Binaryen.js, even though it is likely that similar projects building to ESM would encounter the same problem. Could either:

  1. Revert on the Emscripten side, which doesn't really fix the underlying issue but delays the problem long enough that it doesn't surface in Binaryen.js. Fix can be reverted on this PR.
  2. Fix this properly on the Emscripten side as per this comment. Wait with this PR.
  3. A combination of the two. Workaround for now as suggested here (which is safe, require will just become unused), and afterwards look into what can be done on the Emscripten end, so we can delete the externs-pre file here eventually.

Fine with what you folks decide, just need to know whether to revert the workaround here :)

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.

Thanks!

@kripken kripken merged commit 917abba into WebAssembly:main Sep 22, 2022
@kripken
Copy link
Member

kripken commented Sep 22, 2022

This seems good for binaryen. Yeah, as you said, the general issue remains in emscripten, which we need to figure out - I worry other projects will hit this too. But for binaryen I think we're good now.

kripken added a commit that referenced this pull request Sep 22, 2022
This reverts commit 6cc44fd.

#5075 fixes the require() issue, so we don't need the pin.
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