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

Web REPL cleanups #5863

Merged
merged 3 commits into from
Sep 28, 2023
Merged

Web REPL cleanups #5863

merged 3 commits into from
Sep 28, 2023

Conversation

brian-carroll
Copy link
Contributor

@brian-carroll brian-carroll commented Sep 28, 2023

  • Change the height of the loading message to match the loaded state (screenshots below)
  • Make JS code clearer
  • Remove wasi.js since we never use it
    • There is no way for the user's Roc program to make system calls. The platform we use for the REPL has no effects.
    • When I wrote this originally I don't think I fully understood platforms and effects and stuff. (And gen_wasm was actually generating with WASI imports because I hadn't figured out how to eliminate them.)
    • If we ever support dbg we might need to bring back WASI? There are other ways of implementing it.

Previous loading state

Darker than normal because I paused JS at a breakpoint to take the screenshot
Screenshot 2023-09-28 at 09 14 20

New loading state

Darker than normal because I paused JS at a breakpoint to take the screenshot

Screenshot 2023-09-28 at 09 26 45

Loaded state

Screenshot 2023-09-28 at 09 13 56

@brian-carroll brian-carroll changed the title Web REPL: clarify JS code Web REPL: clarify JS code & fix loading message height Sep 28, 2023
@brian-carroll brian-carroll changed the title Web REPL: clarify JS code & fix loading message height Web REPL cleanups Sep 28, 2023
@rtfeldman
Copy link
Contributor

If we ever support dbg we might need to bring back WASI? There are other ways of implementing it.

I think we should be able to do this using the same way we do roc_alloc, except have it call console.log - so, still shouldn't require wasi!

Copy link
Contributor

@rtfeldman rtfeldman left a comment

Choose a reason for hiding this comment

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

Lovely, thanks @brian-carroll! 🎉

@rtfeldman rtfeldman merged commit eea619e into main Sep 28, 2023
14 checks passed
@rtfeldman rtfeldman deleted the web-repl-clarify-code branch September 28, 2023 10:50
@@ -32,7 +31,7 @@ const repl = {
app: null,

// Temporary storage for values passing back and forth between JS and Wasm
result: { addr: 0, buffer: new ArrayBuffer() },
result_addr: { addr: 0, buffer: new ArrayBuffer() },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow, this is wrong. This field was changed to a number everywhere else.
The initial value always get overwritten, so everything works. But it's confusing.
I'll make another PR.

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