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

npm package should not include .ts files #2393

Closed
mneil opened this issue Apr 11, 2022 · 24 comments
Closed

npm package should not include .ts files #2393

mneil opened this issue Apr 11, 2022 · 24 comments
Labels
bug Something isn't working

Comments

@mneil
Copy link
Contributor

mneil commented Apr 11, 2022

🐛 Bug

I just did an npm i pyodide wanting to use this as part of a projec for packaging AWS Lambda packages to support the Python Lambda Functions. I've been testing out pyodide and brython to see which one makes more sense. I am really liking pyodide. Up to now I've been including the file from a cdn and it's gone fine. However, the package on npm includes all of the typescript files.

An npm package should include type definitions (.d.ts) files for typescript and javascript users for type definitions. The rest of the code, however, should all be javascript. When you publish .ts files I have to now configure whatever build system I'm using (webpack) to support typescript and to explicitly hand transpiling the code. This is not the common way to distribute a package on npm.

In addition there is no main file defined in package.json. I am unsure what the main file is that I should include in order to get the full source.

To Reproduce

npm i pyodide

Expected behavior

When I installed this I expected:

  • Javascript file or files
  • A single file defined as the main so that my requires or imports know what file to use when I request pyodide
  • The asm.js file for executing the pyodide application compiled with emscripten
  • The compiled python wasm that will be executed

I believe all of these files should be included. This will make integration into other packages much easier.

Environment

  • Pyodide Version 0.20.0

Additional context

I am happy to work on this if you agree. Hopefully I didn't misinterpret the point of the package on npm.

I have recently been using esbuild which is written in go and distributes a wasm for bundling. That code is successfully on the develop branch of cdk-web https://github.com/3p3r/cdk-web/tree/develop. I am comparing my experience dealing with the package distributed on npm for esbuild-wasm vs. my experience with pyodide.

@mneil mneil added the bug Something isn't working label Apr 11, 2022
@hoodmane
Copy link
Member

hoodmane commented Apr 12, 2022

Yeah, I agree that we are publishing the wrong stuff. It should be a simple fix: instead of publishing our Javascript source directory which is useless, we should publish the built files. Nobody here knows how to use node all that much so we haven't fixed it.

@hoodmane
Copy link
Member

hoodmane commented Apr 12, 2022

For the record, to get something that actually works right now:

  1. Download and unpack https://github.com/pyodide/pyodide/releases/download/0.20.0/pyodide-build-0.20.0.tar.bz2
  2. you can import {loadPyodide} from "pyodide.mjs" or let {loadPyodide} = require("pyodide.js"). The former is probably better, the latter defines loadPyodide and _createPyodideModule as global variables.
  3. pyodide.d.ts has type definitions

@hoodmane
Copy link
Member

After some exploration, it seems that what will work is to copy package.json into the dist folder, cd into the dist folder and then run npm publish. npm doesn't seem to have any progress indicator but it successfully uploads all 160mb. I published v0.20.0 here:
https://www.npmjs.com/package/hoodmane-pyodide
After a very long download time it... doesn't work very well.

Some issues:

  1. we guess the wrong indexURL
  2. Emscripten uses require which causes "require is not defined" when we load the es6 module

@EricCrosson
Copy link

EricCrosson commented Apr 12, 2022

@hoodmane , I normally use a combination of files1 (example), main2 (because I focus on Node.js packages, frontend has different requirements I'm a bit hazier on) (example), and types3 (when using TypeScript).

The documentation on files is a little confusing, instead of describing from the perspective from the install, I would have said "a whitelist of files included in the published package."

So you can use some value for files that points into the desired part of the dist directory instead of moving the package.json and cding, and be sure that you also define the entrypoint for the npm package with something like main or browser4.

Footnotes

  1. https://docs.npmjs.com/cli/v8/configuring-npm/package-json#files

  2. https://docs.npmjs.com/cli/v8/configuring-npm/package-json#main

  3. https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#including-declarations-in-your-npm-package

  4. https://docs.npmjs.com/cli/v7/configuring-npm/package-json#browser

@hoodmane
Copy link
Member

Okay. Is there some standard way to say "if they try to import an es6 module, take pyodide.mjs but if they try to require us use pyodide.js"?

@EricCrosson
Copy link

EricCrosson commented Apr 12, 2022

Yes there is but I haven't used es6 modules myself.

I believe you just include

  "main": "./dist/src/pyodide.js",
  "module": "./dist/src/pyodide.mjs",

in the package.json. When the npm package is loaded as common js the main entrypoint is used, and when the npm package is loaded as an es module the module entrypoint is used. I could be wrong but that's my understanding

@hoodmane
Copy link
Member

I don't find module documented on npmjs:
https://docs.npmjs.com/cli/v8/configuring-npm/package-json#module
but might as well try it out =)

@EricCrosson
Copy link

I'm not either 😅 I'm seeing

"type": "module"

source

but googling is suspiciously quiet on this. I was reading this article just yesterday that also uses the module technique 🤞

@mneil
Copy link
Contributor Author

mneil commented Apr 12, 2022

Ok, I can help with this as I'm primarily a node / js user but have also used only python for several years. Typically, with typescript, you'd run npx tsc to build. And your tsconfig will have all the settings. You may need multiple tsconfig for different environments like testing, etc...

The built js, package.json, and any wasm files need to be included in that dist folder. Then cd into dist and publish. I can open a pr to resolve some of that and you let me know if I miss anything.

@hoodmane
Copy link
Member

What about if our package.json isn't in the root directory? We could maybe symlink the dist directory into src/js for the npm publish?

@hoodmane
Copy link
Member

Also, our distribution is very large (200mb) but only 15mb is needed to run the interpreter and the rest are packages. It might be good to eventually add some way to split things up a bit so people don't have to download so much stuff if they won't use it.

@mneil
Copy link
Contributor Author

mneil commented Apr 12, 2022

The package. Jason needs to be in the folder with the built js and you can execute publish from that location. Publish builds your archive in the cwd

@hoodmane
Copy link
Member

Okay, so roughly cp package.json $DIST_DIR && cd $DIST_DIR && npm publish?

@hoodmane
Copy link
Member

I'm not either 😅 I'm seeing

"type": "module"

Some experimentation shows that "type" : "module" causes an error if you try to require anything from the package.

@hoodmane
Copy link
Member

hoodmane commented Apr 12, 2022

Okay, after some trial and error, hoodmane-pyodide@0.20.0-g seems to work correctly. I used this branch:
#2394
Both test.mjs:

import { loadPyodide } from "hoodmane-pyodide/pyodide.mjs";

async function main(){
    let pyodide = await loadPyodide();
    pyodide.runPython("print(1+1)");
}
main();

and test.js:

let {loadPyodide} = require("hoodmane-pyodide");

async function main(){
    let pyodide = await loadPyodide();
    pyodide.runPython("print(1+1)");
}
main();

work as expected. It seems like we only get one entrypoint, we can choose whether the commonjs or the es6 entrypoint is preferred, the other has to be imported like "hoodmane-pyodide/pyodide.mjs".

@rth
Copy link
Member

rth commented Apr 12, 2022

we should publish the built files

I know that works in a relatively straightforward way, but I think npm i pyodide shouldn't download all of Pyodide packages. I know we are not paying for bandwidth in npm it's still wasteful and wouldn't scale well if we have much more packages. Maybe we can only include the core pyodide build files (if there is no way around including build files) and let the rest be downloaded from the CDN (or alternatively from other NPM packages).

Besides we are already installing packages from PyPi anyway, so we won't be able to put all dependencies on npm anyway.

@mneil
Copy link
Contributor Author

mneil commented Apr 12, 2022

I believe what you want is to use exports in the package.json to define what file to use depending on the module definition (common vs. esm).

https://www.the-guild.dev/blog/support-nodejs-esm

#package.json

{
  "main": "pyodide.js",
  "exports": {
    "require": "pyodide.js",
    "import": "pyodide.mjs"
  }
}

The module and exports are not official sections yet. However, node looks at the exports field since v12 LTS. Module is used for bundlers like webpack - maybe rollup also looks at modules but I'm not as familiar with it.

@mneil
Copy link
Contributor Author

mneil commented Apr 12, 2022

@rth the package @hoodmane published recently as a test contains roughly 18mb on disk and contains everything I understand is needed to execute pyodide. The wasm binary, pyodide js, asm.js, and web workers. Anything the user installs later via pyodide install or micropip should continue to work as it does currently (pull from pypi or pyodide cdn).

One option could be to run a postinstall script that downloads larger files after the install from a CDN. However, some security focused organizations like to disable post install scripts which could make onboarding pyodide slightly harder. The onus is on those users to handle the post install themselves if they choose to disable it.

I personally think 18mb is large but not massive for an npm packages total footprint. A typical react app is going to pull more than that down when you look at the size of all transient dependencies.

@rth
Copy link
Member

rth commented Apr 12, 2022

Sounds good then, I read the discussion too fast :)

@hoodmane
Copy link
Member

18mb is large but not massive for an npm packages total footprint

And remember what you are doing here is installing Python using npm i. Normal Python is about 24mb. So hopefully people shouldn't be too surprised about this.

@alexmojaki
Copy link
Contributor

Just ran into this while trying to use the pyodide npm package as a dependency in my own package (alexmojaki/pyodide-worker-runner#8).

Nobody here knows how to use node all that much so we haven't fixed it.

My intended environment is a browser, but I can't even run a webpack build. After fighting with configuration a bit, I hit an impasse because the .ts files say things like import ... from "./api.js" and of course there's no api.js, only api.ts.

Also, our distribution is very large (200mb) but only 15mb is needed to run the interpreter and the rest are packages. It might be good to eventually add some way to split things up a bit so people don't have to download so much stuff if they won't use it.

Ultimately I'd like to use the Pyodide npm package for the type declarations and to use loadPyodide to skip the extra network request of an initial importScripts. But I don't want to download the full distribution or serve it myself, I still want to set indexURL to the CDN.

@alexmojaki
Copy link
Contributor

BTW, the final package can still include the .ts source files in addition to the built .js files. This can be used for source maps, by the IDE, or just informally to look at source code in node_modules without having to come to github.

@hoodmane
Copy link
Member

the final package can still include the .ts source files in addition to the built .js files

I think there's some risk of confusion between pyodide.ts and pyodide.js? If you say

import { loadPackage } from "./pyodide";

in a typescript file, won't it use pyodide.ts instead of pyodide.js?

used for source maps

I think we are including source maps.

@hoodmane
Copy link
Member

I can't even run a webpack build ... After fighting with configuration a bit, I hit an impasse because the .ts files say things like import ... from "./api.js" and of course there's no api.js, only api.ts.

Well probably we should fix that too. It would be helpful if anyone who is better at node/webpack/typescript could contribute to #2397. Currently our mocha tests don't even run and I haven't figured out why. #2394 improves the situation a bit.

Maybe we should deploy a release (maybe 0.20.1a1?) with #2394 so people can test out the changes and help us fix any further problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants