-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Improve wasm demo website #230
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
Conversation
@coolreader18 thank you very much for your contribution! I made some comments in your changes. @rmliddle and @shinglyu I am not wasm bindgen expert, can you take a look at this as well? |
eval_py(`return js_vars["a"]`, { a: 9 }) == 9
@windelbouwman Would you mind if used codemirror for the input box and changed the webpack config for the js app? Just copying an html file over to the dist directory isn't really idiomatic webpack, although "idiomatic webpack" sounds like something from /r/programmingcirclejerk. |
@coolreader18 I am no web expert, so I would like to pass this question on to @shinglyu or @rmliddle , for me, any improvement is fine. I would also like for the site to remain relatively simple, so with plain text area and output area. This should be sufficient. Maybe, we can create a second site which uses a more cloud connected way of displaying code. Again, I am relatively web-n00b, so I would like comments from others. Then again, if this takes to long, I will merge this PR. |
Sorry for the delay, I was away for a few days. As for codemirror, I'd rather keep the demo simpler. Because this is a demo for web user and also code readers, so the source code for this demo should be minimal and easy to read. I wouldn't mind creating another website for a more elaborate "RustPython Playground" kind of demo, so we can make it a full-fledged online IDE. The webpack config was also kept minimal for readability. Could you elaborate on how you'd like to make it more idiomatic? |
vm.ctx.set_attr( | ||
&vm.builtins, | ||
"print", | ||
vm.context().new_rustfunc(wasm_builtins::builtin_print_html), |
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.
Nice! I like this dual-function design of print_html + print_console
Mainly just use html-webpack-plugin instead of webpack-copy-plugin. It's fine though, I was mainly thinking of using that because I needed to import codemirror's css and wanted to bundle it in. |
The code looks great overall. Could you also add the documentation for |
This is a completely separated discussion: Do you think it's a good idea to create an unbundled version for debugging and a optimized one for release? I'm terrible at webpack and would love to hear for modern web experts. We can create an issue and move the discussion there. |
Also, switch from iterating over the values of js_injections and serializing each of them individually to asserting it's an object and then just stringifying the whole thing.
This seems ready to merge, so I think this is best left to a separate PR or issue, but: what about splitting the wasm demo into a playground for the website, and an example of how to embed it into JS? That way we could get a good UX with codemirror/whatever on the website, and also provide a simple and readable example if someone wants to embed it themself? Maybe publish the built package to NPM as well, so people aren't dependent on the source tree. |
@coolreader18 Yes splitting the demo sounds like a good plan. Feel free to open an issue to followup. I'll merge this one. Thank you for contributing! |
This pull request:
window.rp
to the RustPython wasm module.eval_py()
function to the wasm module, with printchanged to console.log instead of print_to_html
eval_py("", { a: 9 })
and access them in python with the dictjs_vars
return
a value at the end of the demo script and have it print to the consoleI reorganized the module a little bit as well.