Skip to content

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

Merged
merged 8 commits into from
Dec 21, 2018
Merged

Improve wasm demo website #230

merged 8 commits into from
Dec 21, 2018

Conversation

coolreader18
Copy link
Member

@coolreader18 coolreader18 commented Dec 15, 2018

This pull request:

  • sets window.rp to the RustPython wasm module.
  • adds an eval_py() function to the wasm module, with print
    changed to console.log instead of print_to_html
  • you can pass variables to eval_py("", { a: 9 }) and access them in python with the dict js_vars
  • allows you to return a value at the end of the demo script and have it print to the console

I reorganized the module a little bit as well.

@windelbouwman
Copy link
Contributor

@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?

@coolreader18
Copy link
Member Author

coolreader18 commented Dec 18, 2018

@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.

@windelbouwman
Copy link
Contributor

@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.

@shinglyu
Copy link
Contributor

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),
Copy link
Contributor

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

@coolreader18
Copy link
Member Author

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.

@shinglyu
Copy link
Contributor

shinglyu commented Dec 18, 2018

The code looks great overall. Could you also add the documentation for js_var to the demo page? Maybe under the stdout and stderr textarea so the precious screen real-estate are left to the textareas, and really interested users can scroll down and read it. It would also be nice to document the print-to-html vs print-to-console design in a README file under wasm/, but that is totally optional.

@shinglyu
Copy link
Contributor

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.
@coolreader18
Copy link
Member Author

coolreader18 commented Dec 19, 2018

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.

@shinglyu
Copy link
Contributor

@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!

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.

3 participants