8000 Introduce a Plugin system, implement <py-terminal> as a plugin by antocuni · Pull Request #917 · pyscript/pyscript · GitHub
[go: up one dir, main page]

Skip to content

Introduce a Plugin system, implement <py-terminal> as a plugin #917

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 18 commits into from
Nov 9, 2022

Conversation

antocuni
Copy link
Contributor
@antocuni antocuni commented Nov 3, 2022

This PR does two things:

  1. introduce the first minimal version of a Plugin system. Plugins are subclasses of the Plugin class, and pyscript will call the relevant methods/hooks at the right time. Currently, I have added only the minimal sets of hooks which were needed for this PR.
  2. Implement <py-terminal> as a plugin.

As the name suggests, <py-terminal> implements a (very simple, so far) terminal where stdout and stderr go:
image

This is a good example which shows how plugins have a broader scope than just Custom Elements: the PyTerminal plugins defines the <py-terminal> custom elements, but it also does more: in particular, it automatically add a <py-terminal> tag to the page if it's not present (depending on the config settings).


Operating principles of <py-terminal>

I introduced a way to capture/redirect the stdout and stderr streams used by pyodide: when added to the page, PyTerminal register itself as a Stdio provider, and when lines are written to stdout or stderr, PyTerminal intercepts the call and shows the messages. This ultimately means that calls to print() goes to the terminal.

The terminal can work also in "autoshow" mode: if you put <py-terminal auto>, it will start hidden and reveal itself as soon as the first line of text is written to it.

Moreover, I added a config.terminal setting, which can have three values:

  • true: automatically add a <py-terminal> to the page, if not already present
  • "auto": automatically add a <py-terminal auto> to the page, if not already present
  • false: don't add anything

I chose to make "auto" the default. This is very useful because it leads to the following behavior:

  1. if you only use display() you will never see the terminal
  2. as soon as you do a print(), the terminal shows up. This is much better and intuitive than the current situation in which print() are basically hidden and the only way to see them is to open dev tools.
  3. Corollary of the previous point: this makes sure that if someone is following some random python tutorial which uses print(), it will just work.

Further enhancements

This is intentionally the simplest possible version of a terminal, but it's already useful IMHO. The following is a random list of future enhancements, although I don't have plans to work on them soon (but maybe someone wants):

  • docs: I didn't write any docs, sorry 🤷‍♂️ . Moreover, it would be nice to show a nice message What is it? (or similar) in auto mode: the idea is that it will contain a link to the relevant docs which explains what is it, how to configure, how to turn it off, etc.

  • dockable mode: currently <py-terminal> is automatically added at the end of the DOM. It would be nice to have a way to dock it to the top/bottom/left/right of the page, and maybe to resize it dynamically

  • minimize mode: another interesting alternative UI could be to have a "terminal icon" somewhere on the page, which displays notifications when it gets new messages: if you click on the icon, the terminal shows up

  • better styling: I did the minimal CSS possible, but it's likely that we can do better. For example, it would be nice to automatically get a horizontal scrollbar if the lines are too long, and/or to set a fixed vertical height and display also a vertical scrollbar

  • dumb vs xterm mode: this is a dumb terminal: no ANSI escape codes, no colors, etc. It would be nice to add the option to use xterm.js instead. According to the webpage, it is used a bit everywhere, including visual studio c 8000 ode. This means that for example we could run pytest inside the browser and see all the nice colors and formatting.

  • stdin support: this is harder, and most likely requires to run pyscript in a web worker (because reading from stdin is a blocking syscall). But once we have it, things like e.g. pdb.set_trace() would probably work out of the box! 😱


Enhancements to the Plugin system

As a follow-up of this PR, I plan to migrate to turn the loader and register_importmap into plugins.

@antocuni antocuni marked this pull request as draft November 3, 2022 18:04
@antocuni antocuni changed the title WIP: implement <py-terminal> as a plugin Introduce a Plugin system, implement <py-terminal> as a plugin Nov 7, 2022
@antocuni antocuni marked this pull request as ready for review November 7, 2022 16:13
Copy link
Contributor
@FabioRosado FabioRosado left a comment

Choose a reason for hiding this comment

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

Just had the smallest of comments but this looks good!
I'll open an issue so I dont forget to write the docs for this 😄


configure(config: AppConfig) {
// validate the terminal config and handle default values
const t = config.terminal;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit, but would be nice to have a more discriptive name although the code is small enough to not be too confusing 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm following this rule here. The point is precisely that the code is short, and having a longer name would make it harder to read IMHO.
The reason why I introduce the extra var was to avoid this:

        // validate the terminal config and handle default values
        if (config.terminal !== undefined &&
            config.terminal !== true &&
            config.terminal !== false &&
            config.terminal !== "auto") {
            // XXX this should be a UserError as soon as we have it
            const got = JSON.stringify(config.terminal);
            throw new Error('Invalid value for config.terminal: the only accepted'  +
                            `values are true, false and "auto", got "${got}".`);
        }
        if (config.terminal === undefined) {
            config.terminal = "auto"; // default value
        }

That said, I'm happy to change t into a longer name if you think it leads to more readable code: what do you think it should be?
I'm serious here: an external reviewer is a better judge w.r.t readability than the original author of the code, so happy to hear your suggestions.

Copy link
Contributor
@WebReflection WebReflection Mar 15, 2023

Choose a reason for hiding this comment

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

FYI in that part there is no t anymore :)

@@ -54,8 +58,8 @@ export class PyodideRuntime extends Runtime {
async loadInterpreter(): Promise<void> {
logger.info('Loading pyodide');
this.interpreter = await loadPyodide({
stdout: console.log,
stderr: console.log,
stdout: (msg) => { this.stdio.stdout_writeline(msg); },
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add typing here or does TS knows msg will be string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm, good question, I don't really know.
I tried to compile with --strict and the compiler doesn't give any warning, so maybe somehow it manages to infer it. But adding an explicit type doesn't hurt, so I added it in 095f156

@antocuni
Copy link
Contributor Author
antocuni commented Nov 9, 2022

@FabioRosado I think this is ready to be merged, but I'll let you decide about the naming of t :)

@FabioRosado
Copy link
Contributor

I think it's okay like you said the code is small enough that it doesn't affect understanding of the code

Feel free to hit merge 😄

@antocuni antocuni merged commit 0d3c3ee into main Nov 9, 2022
@antocuni antocuni deleted the antocuni/py-terminal branch November 9, 2022 15:17
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
0