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

ENH Add loadPyodide packages option for loading packages during bootstrap #4100

Merged
merged 7 commits into from Aug 29, 2023

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Aug 28, 2023

For improved loading performance.

  • Add a CHANGELOG entry
  • Add / update tests
  • Add new / update outdated documentation

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM with some minor comments.

return Buffer.from(b16, "hex").toString("base64");
}

function browserBase16ToBase64(b16: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

Is this because btoa is not supported in Node < 16.0?

If so, the lifecycle of Node 14 was ended a few months ago, and Node 16 will also retire soon (in a few weeks), so I think we can think of dropping support for older Node versions to reduce the maintenance burden.

(no action required for this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's officially drop node 14 support after the release?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does say on the node atob api docs:

This function is only provided for compatibility with legacy web platform APIs and should never be used in new code, because they use strings to represent binary data and predate the introduction of typed arrays in JavaScript. For code running using Node.js APIs, converting between base64-encoded strings and binary data should be performed using Buffer.from(str, 'base64') andbuf.toString('base64').

But probably we should reasonably ignore this?

src/py/pyodide/_package_loader.py Show resolved Hide resolved
@hoodmane
Copy link
Member Author

Thanks for the review @ryanking13.

@hoodmane hoodmane merged commit a8f2409 into pyodide:main Aug 29, 2023
0 of 2 checks passed
@hoodmane hoodmane deleted the preload-packages branch August 29, 2023 11:43
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

2 participants