Fix npm package #2394

hoodmane · 2022-04-12T01:23:14Z

When we load pyodide from node_modules, the calculated indexURL
starts with file:// which causes trouble. Truncate it off.

Work towards fixing #2393.

When we load pyodide from node_modules, the calculated indexURL starts with `file://` which causes trouble. Truncate it off.

src/js/rollup.config.js

rth · 2022-04-12T09:16:22Z

src/js/pyodide.ts

-  const fileName = ErrorStackParser.parse(err)[0].fileName!;
+  let fileName = ErrorStackParser.parse(err)[0].fileName!;
+  if (IN_NODE && fileName.startsWith("file://")) {
+    fileName = fileName.slice("file://".length);


This could use an explanatory comment about why we are doing this.

Well we get file://path but node expects paths not to start with file://

OK, and I assume this would also work when importing pyodide js from local files? Could you please add a comment that this is necessary when determining indexURL with remote URLs in node?

I assume this would also work when importing pyodide js from local files?

Well yeah. The path shouldn't ever start with file:// in Node. If it starts with file:// we remove it, if not we leave it alone. I think a sort of related issue is that in compat.ts in node we check if the path starts with https:// in which case we make a web request otherwise we make a fs operation. But the fs operations get mad about paths starting with file://. Maybe a better solution would be to remove this in compat.ts.

src/js/compat.ts

rth

Thanks, a couple more comments otherwise LGTM. Also opened #2397 to improve a bit the quality control of this package before the release.

But yeah, since none of us has much experience with typescript, we are doing what we can :)

src/js/package.json

…works

ryanking13 · 2022-04-16T00:47:10Z

src/js/load-package.ts

+  // If we are IN_NODE, download the package from the cdn, then stash it into
+  // the node_modules directory for future use.
+  let binary = await _loadBinaryFile(cdnURL, file_name);
+  await nodeFsPromisesMod.writeFile(`${baseURL}${file_name}`, binary);


I'm not very sure of this. IMO this kind of behavior should better be more explicit. For example, puppeteer downloads binaries during installation (though that is not what we want because Pyodide is not only for server-side usage), or playwright provides a command playwright install to download binaries after installation.

Maybe take a config variable? My main concern is that packages.json is still going to include all of these packages but the request will fail when it tries to open the file which doesn't exist.

My main concern is that packages.json is still going to include all of these packages but the request will fail when it tries to open the file which doesn't exist.

Yes, I also think it would be great if people don't need to download packages manually. So providing a way of automatically downloading packages would be nice for the user experience.

My opinion is that we should let Pyodide users know what is going on. They need to know that whl files are not included in the npm packages and those will be downloaded after installation.

If we are going to download packages in runtime as in this PR, I think there needs a proper log telling that it is downloading and stashing packages.

Maybe take a config variable

Yeah, a config variable for deciding whether to download missing packages in runtime (or die with error) would be nice IMO. It would also be nice if CDN URLs can be configured at the top level as there were some requests.
Still, I think it would be great to provide an explicit way to install packages before runtime (maybe $ npx pyodide install or something?) and recommend that way. The advantages of downloading them before runtime will be:

If there is a problem in the CDN, it can be checked beforehand.

Running time is stabilized (we don't need to download a large package at the first run)

We can verify checksums of packages beforehand.

maybe $ npx pyodide install or something?

I am not likely to work on this myself too soon, but I am happy if someone else would do it.

In the meantime, I'm not sure what we should do in this PR. If you have an opinion, I would be happy for you to put in code that you like (you can push it directly to this branch). I think I agree with you about the long term solution, but I'm not what the best intermediate solutions are.

I think there needs a proper log telling that it is downloading and stashing packages.

Perhaps adding logging to this for now would be okay? Or we could remove it from this PR and throw an error if a missing package is loaded?

Perhaps adding logging to this for now would be okay?

Yeah I am okay with that for this PR. Perhaps let's not document this behavior for now. Thanks for working on this!

I am not likely to work on this myself too soon, but I am happy if someone else would do it.

I'll open a issue so we can revisit this later.

ryanking13 · 2022-04-18T00:32:53Z

src/js/pyodide.ts

+  if (!pyodide.version.includes("dev")) {
+    // Currently only used in Node to download packages the first time they are
+    // loaded. But in other cases it's harmless.
+    API.setCdnUrl(`https://pyodide-cdn2.iodide.io/v${pyodide.version}/full/`);


Isn't iodide.io url deprecated BTW?

Probably. I still primarily use it...

Perhaps let's use https://cdn.jsdelivr.net as in our documentation.

FYI, we should definitely not use it, it doesn't cache well. I'm thinking we should fully discontinue it soon.

ryanking13

Thanks for working on node issues @hoodmane!

hoodmane · 2022-04-18T04:10:22Z

Thanks for the reviews @rth and @ryanking13!

Various fixes to improve the npm package. Switch to publishing the dist folder, fix indexURL for node, fix node commonJS import and ES6 import only publish core Pyodide interpreter to npm, download and cache other wheels on first use.

hoodmane added 3 commits April 11, 2022 18:22

Fix calculateIndexURL when file is loaded from node_modules

2e56fe4

When we load pyodide from node_modules, the calculated indexURL starts with `file://` which causes trouble. Truncate it off.

Fix 'require is not defined' problem too

83f925e

Update package.json

19068ce

hoodmane mentioned this pull request Apr 12, 2022

npm package should not include .ts files #2393

Closed

hoodmane changed the title ~~Fix calculateIndexURL when file is loaded from node_modules~~ Fix node package Apr 12, 2022

rth reviewed Apr 12, 2022

View reviewed changes

rth reviewed Apr 14, 2022

View reviewed changes

src/js/package.json Outdated Show resolved Hide resolved

src/js/package.json Outdated Show resolved Hide resolved

hoodmane added 15 commits April 14, 2022 11:19

Merge branch 'main' into node-index-url

b6902fd

Handle node + file:// in compat.ts rather than in pyodide.ts

b4a6a3f

Update package.json

078a345

Generate pyodide.d.ts in make all recipe

ec76cce

Copy package.json into dist, adjust recipe for .d.ts file

7e694df

Fix package.json exports section

85c33aa

Cache packages when loaded in node

394a5c5

Add comment

b7cca25

More comments

f44c99e

More updates

3aa7f78

Use npm link for tests so that we actually check whether npm install …

04251ff

…works

Update docs for making a release

27d3aa9

Merge branch 'main' into node-index-url

9f6e828

Fix test-js

af4a26f

Fix js tests a bit

bb71f25

ryanking13 reviewed Apr 16, 2022

View reviewed changes

hoodmane mentioned this pull request Apr 18, 2022

Support importing pyodide as ESM module #2217

Closed

ryanking13 reviewed Apr 18, 2022

View reviewed changes

ryanking13 approved these changes Apr 18, 2022

View reviewed changes

ryanking13 mentioned this pull request Apr 18, 2022

RFC Provide a way to download packages for Node #2402

Closed

Update cdn url, log some info about cdn fetch and node_modules cache

055e0a4

hoodmane changed the title ~~Fix node package~~ Fix npm package Apr 18, 2022

hoodmane merged commit e5a46e5 into pyodide:main Apr 18, 2022

hoodmane deleted the node-index-url branch April 18, 2022 04:10

hoodmane mentioned this pull request Apr 18, 2022

Node package improvements #2403

Open

rth mentioned this pull request May 4, 2022

CI Don't cache dist/node_modules #2478

Closed

rth mentioned this pull request Sep 17, 2022

Add CLI to fetch pyodide release pyodide/pyodide-cli#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix npm package #2394

Fix npm package #2394

hoodmane commented Apr 12, 2022

rth Apr 12, 2022

hoodmane Apr 12, 2022

rth Apr 14, 2022

hoodmane Apr 14, 2022

rth left a comment

ryanking13 Apr 16, 2022

hoodmane Apr 16, 2022 •

edited

Loading

ryanking13 Apr 18, 2022

hoodmane Apr 18, 2022 •

edited

Loading

ryanking13 Apr 18, 2022

ryanking13 Apr 18, 2022

hoodmane Apr 18, 2022

ryanking13 Apr 18, 2022

rth Apr 18, 2022

ryanking13 left a comment

hoodmane commented Apr 18, 2022

Fix npm package #2394

Fix npm package #2394

Conversation

hoodmane commented Apr 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoodmane Apr 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoodmane Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanking13 left a comment

Choose a reason for hiding this comment

hoodmane commented Apr 18, 2022

hoodmane Apr 16, 2022 •

edited

Loading

hoodmane Apr 18, 2022 •

edited

Loading