Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-136251: Improvements to WASM demo REPL#136252
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
….worker.mjs can use it
python-cla-botbot commentedJul 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
All commit authors signed the Contributor License Agreement. |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
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.
not a full review, just saw some style problems :)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
thanks,@sobolevn! i just ran |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Uh oh!
There was an error while loading.Please reload this page.
The demo does not work on Firefox! |
I just now tested on both Firefox 140.0.4 and 128.11.0esr, and it seems to be working for me, both the demo I linked above and a completely-fresh local build. What version of Firefox are you running? And can you provide any more specifics about the way in which the demo is not working for you? |
128.11.0esr here (debian) with advanced third-party tracking protection, ublock, privacy badger… ACE code editor works, but nothing happens when I try to run code. The js console shows: |
That's remarkably similar to my setup, so I'm surprised that it isn't working...but I guess depending on what you mean by "advanced third-party tracking protection," it's possible that you're ending up with Maybe it's worth adding an error message if we notice that |
On the page, window.crossOriginIsolated is false and WorkerGlobalContext undefined |
Right. I guess what I'm thinking currently is that this is likely not an issue with the demo not playing nicely with Firefox per se, but maybe with your specific configuration. For example, I'm running on Debian with "Strict" privacy mode, ublock origin, privacy badger, canvas blocker, etc., but the demo is working for me on multiple machines. I'm wondering what the difference could be...do you maybe have |
Did you use |
| @@ -2334,7 +2334,7 @@ AS_CASE([$ac_sys_system], | |||
| dnl Include file system support | |||
| AS_VAR_APPEND([LINKFORSHARED], [" -sFORCE_FILESYSTEM -lidbfs.js -lnodefs.js -lproxyfs.js -lworkerfs.js"]) | |||
| AS_VAR_APPEND([LINKFORSHARED], [" -sEXPORTED_RUNTIME_METHODS=FS,callMain,ENV"]) | |||
| AS_VAR_APPEND([LINKFORSHARED], [" -sEXPORTED_RUNTIME_METHODS=FS,callMain,ENV,HEAPU32"]) | |||
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.
Oops.
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.
Can you provide more information? I made this change sincepython.worker.mjs is currently trying to look inHEAPU32 to find Python's version info but it wasn't accessible (in my original builds, I just manually editedpython.worker.mjs to hard-code the version number to make things work).
I'm guessing from "Oops," though, that this isn't the right way to fix this? First time messing with Emscripten/WASM at all, so if you don't mind, more context would be appreciated.
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.
@hoodmane The "oops" comment is ambiguous to me as well - just to confirm, this is an "oops, HEAPU32 should definitely be included, how did I miss that?", rather than or "oops, this shouldn't be included"?
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'm sorry for the unclear communication. Change is correct.
this is an "oops, HEAPU32 should definitely be included, how did I miss that?"
Exactly.
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.
Sorry for the confusion; I definitely initially interpreted it the other way 😅 But glad to know I wasn't making a stupid mistake!
I used the published demo linked in the PR description, not local server. Let’s ignore my issue for now then… |
🫤 hmm...i'm definitely still a little bothered that it isn't working for you (and really curious what the issue is!). hopefully we'll be able to get some more datapoints. and regardless, thanks for the testing and code review so far! |
@merwok was it working for you before this change? |
@merwok, if it helps, here is a build from before I made my changes:https://hz.mit.edu/wasm_repl_demo_old/python.html. That one only has two changes from the main branch, required to make the demo work at all: closing the I'm pretty confident that I didn't change anything here that would affect Firefox's ability to load the page, but certainly worth checking. That said, I would also be interested to know whether a local build works for you if you're able to give that a try, in case there's something weird about how my machine is serving things (though I do think I'm sending the right headers). If it's desirable, I can also add some kind of error message to the demo in the case where |
hoodmane commentedJul 11, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I agree.
Please do. Let's add a link to this:https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer#security_requirements |
| this.xterm.write( | ||
| "\x1b[" + (this.cursorPosition + 4) + "D", | ||
| ); |
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.
Native console also writes outKeyboardInterrupt when you pressCTRL+C, why not do that here too?
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 can definitely add that. I had thought about it initially but felt like it was awkward for me just to print that when Python wasn't actually ever seeing theCTRL+C. But I think you're right that it would be good to mimic what people see at the normal REPL. I can make that change.
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.
TheCTRL+C handling is actually not really working correctly in my current implementation, unfortunately. Its behavior matches that of the regular REPL if we're dealing with single-line inputs only, but in multiline method, it doesn't match (my implementation results in the code being run instead of ignored). I'd like to actually send aSIGINT in the case of receiving aCTRL+C, but I haven't yet figured out how to do that.
If I'm not able to find a way to make that actually work the same way the regular REPL does, the question is whether we just accept this limitation and leave it as-is, or remove theCTRL+C case entirely.
| writeLine(line) { | ||
| this.xterm.write(line.slice(0, -1)); | ||
| this.xterm.write("\r\n"); | ||
| return line; | ||
| } | ||
| handleCursorInsert(data) { | ||
| this.input += data; |
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.
What'shandleCursorInsert() do?
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's responsible for handling the case where we inputted a printable character (when a keypress doesn't get caught by one of the special handlers, it falls down to this function).
| this.input = this.input.slice(0, -1); | ||
| this.xterm.write("\x1B[D"); | ||
| this.xterm.write("\x1B[P"); | ||
| const trailing = this.input.slice(this.cursorPosition); |
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.
What's this change do?
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.
This is part of the backspace handler (though there's similar code for adding a character, and for the delete key). Now that we have the ability to use the arrow keys to move around within a line, we need something like this to make sure that if we backspace/delete/insert in the middle of a line we can still see all of the characters that come after the one we just deleted.
| @@ -269,6 +462,16 @@ | |||
| } | |||
| return new Promise((resolve, reject) => { | |||
| this.resolveInput = (value) => { | |||
| if (value.replace(/\s/g, "").length != 0) { | |||
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.
Ideally it would be nice to save the history when refreshing. Maybe add a "TODO"? (Not to imply that anyone will ever do it.)
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's definitely easy to change things so that the history persists if you do multiple iterations of "Start REPL" followed by "Stop". I actually kind of went out of my way to clear the history between REPL sessions.
Saving history across reloads also probably wouldn't be too terribly hard if we wanted that; we could shove the history intosessionStorage so that it would persist through a regular refresh but get cleared on closing the tab or (I believe) on a hard reload of the page. Happy to give that a shot if you think it's worthwhile.
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 think saving the history is nice... particularly combined with reverse search! But it's up to you if you want to bother.
hoodmane left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Tried it out and it works great. Thanks! Left some minor comments/questions.
Great; thanks for taking a look! I'll make some of those changes and ping here again when it's ready for another look. |
That’s useful, thanks! the older demo (that you published) has the same issue for me. For running locally, I have a 3.13 build that’s a few months old, but have never done the wasm/emscripten build, and probably won’t have the time to look into it. |
Sorry for the delay here,@hoodmane ! I just pushed a few changes. I think I'm pretty happy with where things are right now, but definitely open to more feedback / suggestions / bug reports! @merwok, if you're interested to take another look, I also updated the copy of the demo on my server:https://hz.mit.edu/wasm_repl_demo/ Here's what changed:
|
Uh oh!
There was an error while loading.Please reload this page.
I'm including a couple of fixes for the WASM demo, which for me made the demo "just work" out of the box for me with a build from following the instructions onthis page:
HEAPU32at build time sincepython.worker.mjsexpects it to be there (for determining the version number)xterm.jsI also made a few changes to try to improve the usability of the web REPL, also included here:
textareawithAce code editor.index.htmlinstead ofpython.htmlso that the URL shown in the terminal when runningserver.pytakes you to the demo (without needing to append/python.html)I also put up a demohere that shows the result of a fresh Emscripten build with these changes.
There's definitely still room for improvement here (for example,#124621 suggests updating the demo to use PyREPL), but I'm sharing these changes in case they're useful in the meantime.
Related Issue:#136251