Support for class-based workers

[winniederidder]

I'm currently trying to integrate the package, but I don't feel like it's fully suited to work well together with classes used in the worker. When using a class, the extra arguments such as syncExtras, the interruptbuffer, ... are passed to every call even though they can be stored in the class and accessed via this. To support it in the current state, every method in your worker class would need to have those arguments at the front of the argument list, which is quite inconvenient. I think providing a base class in this package that stores the extra arguments locally and calls the exposed functions normally would be a good addition to make this package more user-friendly.

[alexmojaki]

Thanks for the suggestion. I'm not sure this would be more user friendly, as it would complicate the API and add more to learn and get confused about. But I'll keep this in mind and see how the Papyros code looks.

At the moment, each syncExtras is built from a fresh callback sent by the client, and the callbacks are tied to a single call so that they can ignore messages after that call ends. I know there are other ways to achieve that, like the UUIDs used in Papyros, but the point is that making these things reusable would require some thought.

I only really expect you to pyodideExpose one method, the one that runs code. Do you have several for that now for debugging or something? Or are you also using this to allow interrupting auto-completion?

[winniederidder]

I hadn't looked at it from that perspective. I was trying to wrap the entire worker in the expose-call, instead of exposing only the methods that require interrupting specifically. I thought I could no longer call methods on the proxy directly and would have to use syncClient.call all the time, so this simplifies some things. Being able to interrupt autocompletion might also prove worthwhile, so having this option available would be nice.

One issue however still is that e.g. my class structure issue kind of still stands. For instance, a Backend has a method runCode. The JavaScript version implements runCode and is exposed using syncExpose. The Python-version uses pyodideExpose, which has a different argument set, making base+child classes have differing types for the same methods, which isn't convenient and requires some casting to any to work.

[alexmojaki]

What do you think of removing the extra pyodide argument supplied by pyodideExpose after syncExtras? It doesn't really need to be there. It makes some code in futurecoder slightly better, but the incompatibility with comsync seems like a significant issue.

[winniederidder]

Moving it to the back would make it compatible with any API as an optional argument, so users can still access it if they want it. That would still cause issues with the override keyword sadly.

[winniederidder]

Might indeed be better to omit the Pyodide argument entirely and ask the user to store it somewhere. They needed it to call initPyodide themselves (and I don't think it is desirable to call initPyodide every time a pyodideExposed function is called?). Then this can be passed as an argument to pyodideExpose to set the interruptbuffer instead of awaiting the promise and passing it afterwards. What do you think? Exporting the default loader from the library will be helpful if the user only wants the basic one (it is currently not exported).

[alexmojaki]

The extra argument(s) are at the front so that exposed functions can still use variadic arguments at the end.

Would you mind making a PR that removes the pyodide argument that's passed to the user function? As for the argument passed to pyodideExpose, I think ideally it should allow either a promise or the pyodide object itself.

Exporting the default loader from the library will be helpful if the user only wants the basic one (it is currently not exported).

Good idea. I think it should also accept a version argument, defaulting to 0.19.1.

[winniederidder]

The extra argument(s) are at the front so that exposed functions can still use variadic arguments at the end.

Yeah, I forgot that if we alter the variadic arguments, users have to take care to check if the last argument is Pyodide, which is also inconvenient.

I'll incorporate both changes in a PR for tomorrow (pyodide argument and version).

[alexmojaki]

Woops, I merged that a bit too confidently.

Accept the collaborator invite, make a branch instead of a fork so that CI can run, and then fix https://github.com/alexmojaki/pyodide-worker-runner/blob/master/test/worker.ts because it still expects the pyodide argument.

[winniederidder]

Yeah, I just realised that I had forgotten the tests. Will fix them now.

[alexmojaki]

Published 0.0.7 to npm