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

full nodejs support in file_packager.py #164

Closed
wants to merge 38 commits into from

Conversation

gabrielfreire
Copy link

@gabrielfreire gabrielfreire commented Sep 13, 2018

Partially address #14

Changes

  • where it sets the Module var Module = ..., added a check to look for process[%(EXPORT_NAME)s] which is where PyodideNode.js stores its pyodide Module.

  • where it tries to set PACKAGE_PATH, added support for nodejs and stopped throwing errors if not in a browser, now the error is thrown if the user is on some mysterious environment that is not browser, worker or nodejs.

  • changed the fetchRemotePackage method to check for XMLHttpRequest before trying to fetch anything, if user is not on a browser, use nodejs isomorphic-fetch/fs to fetch remote/local packages

  • after the data has been fetched, there are some problematic asserts for nodejs, sometimes the buffer is not the arrayBuffer, but the arrayBuffer.buffer

  • it was necessary to change from
    line 700 assert(arrayBuffer instanceof ArrayBuffer, 'bad input to processPackageData');
    to
    assert((arrayBuffer instanceof ArrayBuffer || arrayBuffer.buffer instanceof ArrayBuffer), 'bad input to processPackageData');

  • and from line 701 var byteArray = new Uint8Array(arrayBuffer);
    to var byteArray = new Uint8Array(arrayBuffer instanceof ArrayBuffer ? arrayBuffer : arrayBuffer.buffer);

this format was tested using PyodideNode.js on pyodide.asm.data.js and numpy.js and works fine.

@mdboom mdboom self-requested a review September 14, 2018 02:11
@mdboom
Copy link
Collaborator

mdboom commented Sep 14, 2018

Thanks for the pull request!

I think the test is failing because it's outputting the new anonymous function => syntax. We need to stick to ES5 so the javascript can be optimized through emscripten's pipeline.

Copy link
Collaborator

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

This is awesome. It would be nice to integrate your PyodideNode.js changes, too, when they are ready.

tools/file_packager.py Outdated Show resolved Hide resolved
tools/file_packager.py Outdated Show resolved Hide resolved
tools/file_packager.py Outdated Show resolved Hide resolved
tools/file_packager.py Outdated Show resolved Hide resolved
@gabrielfreire
Copy link
Author

This is awesome. It would be nice to integrate your PyodideNode.js changes, too, when they are ready.

Sure, i can start working on src/pyodide.js to detect the environment and adapt

@mdboom
Copy link
Collaborator

mdboom commented Sep 20, 2018

Thanks for updating this -- it looks like it's passing now.

Should we merge this now, or wait on the corresponding changes to pyodide.js?

Also, can you please rebase on master, rather than merging master? These merge commit will make history much harder to follow.

@gabrielfreire
Copy link
Author

Should we merge this now, or wait on the corresponding changes to pyodide.js?

Merge, please. I'm still trying to figure out the best way to make a clean pyodide.js integration with node, i was thinking in publishing an npm package where someone would npm install pyodide and import something specific for their environment i.e

  • import * as pyodide from "pyodide/web"
  • import * as pyodide from "pyodide/node"

Also, can you please rebase on master, rather than merging master? These merge commit will make history much harder to follow.

yes, sorry about that

@gabrielfreire
Copy link
Author

Hi @mdboom, I did the rebase with your master, but i think it came with something wrong regarding the firefox tests.

@mdboom
Copy link
Collaborator

mdboom commented Sep 21, 2018

@rth: Does anything immediately come to mind for you with the failing test? It's in the new "load from a second origin" test.

@rth
Copy link
Member

rth commented Sep 21, 2018

Thanks for working on this @gabrielfreire !

It looks like the merge master + rebase didn't go so well -- there is a number of commits already on master in this branch. One possibility could be to create a new branch from upstream master, and git cherry-pick your commits there, to get a clean history, then open a new PR. Though maybe there is an easier way around this.

Also could you provide some instruction how one would use this in node? (i.e. what I need to do after launching node to get pyodide there).

Regarding the failing test, it may be an error when closing the connection with the web-server (we get those sometimes in general, it's just than now that are captured in the web-server logs) -- possibly re-running the build would fix it. But this may also mean that the test in question is a bit brittle.

@gabrielfreire
Copy link
Author

Also could you provide some instruction how one would use this in node? (i.e. what I need to do after launching node to get pyodide there).

The node support here is just for the data.js files generated by file_packager.py, it is the 1st phase of the integration, to get this running on node the user would need to require pyodide from PyodideNode.js inside a node js application, and this would require pyodide.js to support nodejs too, which i'm still trying to figure out how to do that without having 1000 lines pyodide.js file, my thoughts are that as the project grows we would need to separate some stuff here, pyodide.js is already kind of big and deals with a bunch of stuff, if we could separate the package loader from the language loader for example, it would be great, maybe put the fixRecursionLimit method in another Utils,js file for example.
Having some project with a structure like this:

  • src/index.js
  • src/packageLoader/PackageLoader.js
  • src/languageLoader/LanguageLoader.js
  • src/Utils/recursionTreat.js
  • src/Utils/....js

the way it is structured now, pyodide.js would get even bigger and harder to maintain.
A new structure would even make the use of Typescript possible, we could build/bundle everything using Webpack and maybe publish to npm as a standalone package to fetch all webassembly related stuff remotely, no matter if you're in a browser or nodejs.

What are your thoughts about this ?

@gabrielfreire
Copy link
Author

Hi guys @mdboom @rth ...
i created another branch, cherry picked and made a new PR

#183

@rth
Copy link
Member

rth commented Sep 26, 2018

Should we close this, as it was superseded by #183 ?

@gabrielfreire
Copy link
Author

Should we close this, as it was superseded by #183 ?

i think you can close this one

@rth rth closed this Sep 26, 2018
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.

None yet

3 participants