Skip to content
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

Split off load-pyodide.js from pyodide.js #1578

Merged
merged 2 commits into from
May 5, 2021

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented May 5, 2021

I moved the package loading code from pyodide.js into a separate file without other changes.
More specifically I changed let loadScript; ==> export let loadScript; and I added import { Module } from "./module"; to the top of the file. Also everything got de-indented.

@rth
Copy link
Member

rth commented May 5, 2021

I was wondering should we try to get to a somewhat similar code organization to #792 to be able to view diffs with that PR or not ? There have been lots of work put into it, and I'm not sure what would be the best way to take advantage of it, even it we don't merge it fully.

@hoodmane
Copy link
Member Author

hoodmane commented May 5, 2021

I think it makes more sense to put the package loading system in its own file than to keep it in the main file since it is a complicated subsystem with an easily identified boundary.

It's a bit hard to incorporate the changes from #792 at this point: they were it was a very large diff when it was first created, and our incremental improvements since has caused a large drift.

The eventual goal of removing global state and using a loader class is reasonable. Probably what I'd do for that is to switch to using this instead of Module in the package loading code and then we could use Module[api_name] = api.bind(Module) in make_public_api.

Anyways the practical matter of keeping diffs manageable seems to mandate doing it in appropriately sized steps.

@hoodmane
Copy link
Member Author

hoodmane commented May 5, 2021

Of course an important blocker for being able to load multiple copies of Pyodide is lack of support in Firefox for modules in webworkers.

@hoodmane
Copy link
Member Author

hoodmane commented May 5, 2021

Yeah I'm not particularly fond of the organization in #792: I want files to be organized by functional subsystem. A util file can be okay if necessary, but why not just put these functions in the same file with the systems they are used in?

@rth
Copy link
Member

rth commented May 5, 2021

I don't have strong convictions for the function organization and both would work for me.

My concern is more are we saying in that PR "thanks for the contribution but the diff is too large and we are not going to be able to accept it and will close it". Or are we still going to try to first iteratively include improvements from that PR before there is more drift.

It would have been nice to get gzuidhof more involved into this design discussions as well. I'm not really a JS developer, I can review such PRs but I don't have enough have hindsight to follow long ranging usability implications (and I also don't have that much availability at the moment). That's why at this point, I'm more concerned about getting more people involved into the review of pyodide-js library from people with solid JS experience and are actually using it rather than implementing any particular feature :)

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

In general, this change looks good to.

I'm more thinking out loud how to get more people involved :)

@hoodmane
Copy link
Member Author

hoodmane commented May 5, 2021

Makes sense.

"thanks for the contribution but the diff is too large and we are not going to be able to accept it and will close it"

Certainly there is no way that we can ever merge #792 but it is nonetheless quite useful. Maybe:

We stole as many ideas as we could understand from this, would welcome smaller improvements / suggestions based on a more up-to-date version of the code.

@hoodmane
Copy link
Member Author

hoodmane commented May 5, 2021

@gzuidhof seems interested in continuing to contribute. And in general we should try to solicit feedback about what aspects of the design are inconvenient for downstream.

I think the changes we've made and are making to the source should help a lot with contributions.
The confusing part of loadPyodide is only about 75 lines worth of code, but it's currently knotted together with everything else.

If we have logically laid out, manageably sized, well documented files, people will hopefully be more likely to contribute improvements.

@hoodmane hoodmane merged commit 8d7659b into pyodide:main May 5, 2021
@hoodmane hoodmane deleted the rollup-split branch May 5, 2021 18:30
@rth rth mentioned this pull request May 13, 2021
hamlet4401 pushed a commit to tytgatlieven/pyodide that referenced this pull request Jul 3, 2021
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.

2 participants