-
Notifications
You must be signed in to change notification settings - Fork 3
Implement support for the remaining top-level exports. #96
Conversation
01593b1
to
fc238e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it looks good to me 👍
- One comment on adding code comment on
genDelayedTopLevelExport
- Comment on
test-exports.mjs
@@ -987,6 +988,19 @@ class WasmBuilder { | |||
ctx.addExport(exprt) | |||
} | |||
|
|||
private def genDelayedTopLevelExport(exportedName: String)(implicit ctx: WasmContext): Unit = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private def genDelayedTopLevelExport(exportedName: String)(implicit ctx: WasmContext): Unit = { | |
/** Generates a delayed top-level export, that is a global variable that is initially set to a | |
* null reference and be later assigned a value in start function. | |
*/ | |
private def genDelayedTopLevelExport(exportedName: String)(implicit ctx: WasmContext): Unit = { |
Can you add a little bit of explanation?
@JSExportTopLevel("SimpleObject") | ||
object SimpleObject extends js.Object { | ||
val bar: String = "the bar field" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to keep Sample.scala
and run.mjs
as it is.
What do you think about having these JSExportTopLevel
s in Test
scope of sample
project like SampleTest.scala
and run test-exports.mjs
using the ./sample/target/scala-2.12/sample-test-fastopt/main.mjs
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge this PR with this, and fix it later of course ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, you're right. That wasn't a great idea. Using the Test scope doesn't work because it would launch the test bridge as part of its start
function, and we don't want that either.
I instead added a bit more infrastructure in tests/test
so that we can test the resulting module exports of a test in test-suite
. Is that better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I instead added a bit more infrastructure in tests/test so that we can test the resulting module exports of a test in test-suite. Is that better?
Yeah, this is awesome, thanks!
fc238e1
to
42137f1
Compare
No description provided.