-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
…r to some arbitrary object
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.
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; |
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.
Just a nit, but would be nice to have a more discriptive name although the code is small enough to not be too confusing 😄
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 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.
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.
FYI in that part there is no t
anymore :)
pyscriptjs/src/pyodide.ts
Outdated
@@ -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); }, |
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.
Do we need to add typing here or does TS knows msg
will be string?
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.
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
@FabioRosado I think this is ready to be merged, but I'll let you decide about the naming of |
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 😄 |
This PR does two things:
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.<py-terminal>
as a plugin.As the name suggests,

<py-terminal>
implements a (very simple, so far) terminal wherestdout
andstderr
go: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 aStdio
provider, and when lines are written to stdout or stderr, PyTerminal intercepts the call and shows the messages. This ultimately means that calls toprint()
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 presentfalse
: don't add anythingI chose to make
"auto"
the default. This is very useful because it leads to the following behavior:display()
you will never see the terminalprint()
, the terminal shows up. This is much better and intuitive than the current situation in whichprint()
are basically hidden and the only way to see them is to open dev tools.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) inauto
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 dynamicallyminimize 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
vsxterm
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 runpytest
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.