8000 support different pyodide versions by fpliger · Pull Request #328 · pyscript/pyscript · GitHub
[go: up one dir, main page]

Skip to content

support different pyodide versions #328

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 11 commits into from
May 11, 2022

Conversation

fpliger
Copy link
Contributor
@fpliger fpliger commented May 11, 2022

Fixes #172

@fpliger fpliger added tag: environment Related to Python environment configuration status: ready PR that is ready for review labels May 11, 2022
@fpliger fpliger requested a review from marimeireles May 11, 2022 04:07
@@ -14,6 +14,11 @@
- paths:
- ./utils.py
</py-env>
<py-config>
Copy link
Contributor

Choose a reason for hiding this comment

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

i like this pattern

Copy link
Contributor
@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

Okay, sorry for taking so long, this ping got lost in my email box.
I've tested with all examples! Seems to work fine.
One thing though is that I'm getting:

(!) Plugin typescript: @rollup/plugin-typescript TS2488: Type 'unknown' must have a '[Symbol.iterator]()' method that returns an iterator.
src/components/pyconfig.ts: (157:48)

157             this.values = Object.assign({}, ...loadedValues);
                                                   ~~~~~~~~~~~~

(!) Plugin typescript: @rollup/plugin-typescript: Rollup 'sourcemap' option must be set to generate source maps.

While running the server. I don't know if it's relevant...? Cause as I said all is working smoothly.

@@ -52,4 +174,17 @@ export class PyConfig extends BaseEvalElement {
close() {
this.remove();
}

loadRuntimes(){
console.log("Initializing runetimes...")
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? runetimes

@@ -1,76 +1,5 @@
<script lang="ts">
import Tailwind from './Tailwind.svelte';
import { loadInterpreter } from './interpreter';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this moved? So it's easier to use with the new array of runtimes that should be on the pyconfig? Or just in general this is the wrong place for this kind of config?

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 removed it because the whole logic was moved to pyconfig.ts and not used here anymore, if needed, other parts can import directly from ./interpreter

The reason I moved to pyconfig was that it helps isolate the whole logic around declaring and initializing runtimes somewhere more contained than just the App. Not sure if pyconfig is the best place for it to be, but it's better than before.

I think there's less and less need for us to have the svelte app... and this also helps us understand how much we depend on it. Atm, afaict, we are only relying on its stores.

@fpliger
Copy link
Contributor Author
fpliger commented May 11, 2022

Thanks @marimeireles , will fix the issues you pointed out before merging. The one related to the Type 'unknown' must have a '[Symbol.iterator]()' method that returns an iterator. src/components/pyconfig.ts is a warning and shouldn't be blocking (although we should make an effort to reduce the warning to 0 at some point) :)

@fpliger fpliger merged commit 363f375 into main May 11, 2022
@fpliger fpliger deleted the fpliger/172_support_different_pyodide_versions branch May 11, 2022 21:59
@fpliger fpliger added status: accepted PR that has been reviewed and accepted and removed status: ready PR that is ready for review labels May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepted PR that has been reviewed and accepted tag: environment Related to Python environment configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to support different version of Pyodide
3 participants
0