Refactor py-config and the general initialization logic of the page #806
Conversation
…check that the <py-config> tag actually works
…store per-app data (e.g. the config)
…T a web component
…omElement instead of going through PyConfig.connectedCallback
…ad of reusing always the same one
…nually instantiated and called by main
… access the confing from Runtime, we explicitly pass it in the constructor
|
I don't really understand why |
There was a problem hiding this comment.
This is cool! I added a comment about the customElements, let me know if you need context on this and some questions mostly for my understanding.
Hope you don't mind them 😄 I tried to look into the pre-commit but the results were truncated so I can't see much info there 🤔
pyscriptjs/src/main.ts
Outdated
| loadConfig() { | ||
| // find the <py-config> tag. If not found, we get null which means | ||
| // "use the default config" | ||
| // XXX: what happens if we have multiple ones? |
There was a problem hiding this comment.
This is likely a bad idea (and you probably thought about it already).
Since loadConfigFromElement takes in an element, what if we do a queryAllSelector and call load config on each item? (I'm also wondering if this will cause some weirdness if one config overrides a previous one 🤔 )
There was a problem hiding this comment.
For simplicity, my feeling is we should do something obvious like enforce only a single <py-config> and raise an error in all other situations.
There was a problem hiding this comment.
yes, probably the comment is poorly worded. What I wanted to say is "I know it's an issue but I don't want to fix it now". I improved the comment in c65a8ba
There was a problem hiding this comment.
For simplicity, my feeling is we should do something obvious like enforce only a single and raise an error in all other situations.
I agree.
@ntoll do you think I should do that in this PR or we can leave it as an exercise for a subsequent one?
There was a problem hiding this comment.
I'd say do it now. Strike while the iron's hot.
Co-authored-by: Fábio Rosado <fabiorosado@outlook.com>
I fiddled with this for a bit yesterday and couldn't work it out... pre-commit tests also pass when run manually on my test environment (Ubuntu 22.04 VM), as does any amount of manually running The pre-commit eslint mirror updated from eslint 8.23.1 to 8.24 last week, so I was wondering if it was an environment issue one or another... but that didn't lead anywhere, clearing reinstalling the precommit hooks and eslint locally still had the tests passing... |
pyscriptjs/src/main.ts
Outdated
| loadConfig() { | ||
| // find the <py-config> tag. If not found, we get null which means | ||
| // "use the default config" | ||
| // XXX: what happens if we have multiple ones? |
There was a problem hiding this comment.
For simplicity, my feeling is we should do something obvious like enforce only a single <py-config> and raise an error in all other situations.
| loadRuntimes() { | ||
| logger.info('Initializing runtimes'); | ||
| for (const runtime of this.config.runtimes) { | ||
| const runtimeObj: Runtime = new PyodideRuntime(this.config, runtime.src, |
There was a problem hiding this comment.
We'll need to figure out the multiple-interpreters story for this... e.g. defining what interpreter to use and then how to instantiate it.
There was a problem hiding this comment.
I agree. Personally, I would like to change the config to allow only one runtime instead of having a list-which-is-not-really-a-list-because-it-doesnt-work-if-you-have-more-than-one.
But as I said above, in this PR I tried to move around the logic without actually trying to change it too much.
There was a problem hiding this comment.
KISS (keep it simple, stupid)... totally agree. Also, if you need more than one runtime, you're probably doing it wrong.
exactly, this is what happens to me as well :(
ah, that's interesting, it seems that the pre-commit config was updated by #794. |
still failing |
There was a problem hiding this comment.
Weird that CI / eslint is failing. We can fix that in another change. But glad to see this getting into the code. 
|
If you run
While I love eslint, it is almost always late. I think we should add But let us save that for a dedicated PR. There are 256 warnings in the current build of tsc, the warnings are hidden behind |
This is a follow-up of PR #806 and it's a big step forward towards solving issue #763. The basic idea is that at this stage I want to streamline the execution logic and make it as linear and easy to read/understand as possible. The diff is relatively big, but for the most part is just "shuffling code around". Svelte stores: the idea is to eventually kill of them, so that we can remove the dependency on svelte but also to avoid relying on so much global state, which makes things more complicated and error-prone (e.g., we have several issues related to using runtime when it's not ready yet). I killed addInitializer, addPostInitializer and the corresponding svelte stores tada. They are no longer needed since the relevant code is called directly from main.ts. I started to kill the usage of the runtimeLoaded svelte store: instead of relying on a global variable, I want to arrive at the point in which the runtime is passed as a parameter in all places where it's needed: pyscript.ts is now free of global state, but I couldn't kill it yet because it's used heavily by base.ts and pyrepl.ts. I will do it in another PR. Other misc changes: I added sanity checks (and corresponding tests!) which complain if you specify 0 or multiple runtimes. Currently we support having one and only one, so there is no point to pretend otherwise I modified the messages displayed by the loader, to be more informative from the user point of view.
This PR is the first step to improve and rationalize the life-cycle of a pyscript app along the lines of what I described in #763 .
It is not a complete solution, more PRs will follow.
Highlights:
py-configis no longer a web component: the old code relied onPyConfig.connectedCallbackto do some logic, but then if no<py-config>tag was present, we had to introduce a dummy one with the sole goal of activating the callback. Now the logic is much more linear.pyconfig.tsonly contains the code which is needed to parse the config; I also moved some relevant code fromutils.tsbecause it didn't really belong to itPyConfigclass did much more than dealing with the config: in particular, it contained the code to initialize the env and the runtime. Now this logic has been moved directly intomain.ts, inside the newPyScriptAppclass. I plan to refactor the initialization code in further PRsPyScriptAppclass, which will hold all the mutable state of the page. Currently onlyconfigis stored there, but eventually I will migrate more state to it, until we will have only one global singleton,globalAppappConfigsvelte store: one less store to kill :).