Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
henryiii
force-pushed
the
henryiii/fix/pyodideload
branch
from
6ca4c71 to
e888c5d
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the repo-review webapp so it no longer relies on the host page (or docs pages) to pre-load Pyodide via a <script> tag, instead loading Pyodide from the CDN at runtime within the app.
Changes:
- Load Pyodide dynamically from jsDelivr at runtime in the webapp utility code.
- Remove Pyodide
<script>tags from the app HTML entrypoint and documentation pages/examples. - Enable JSON imports in TypeScript config and move
pyodideto runtime dependencies to support version-based CDN loading.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Enables resolveJsonModule to allow importing pyodide/package.json for versioning. |
| src/repo-review-app/utils/pyodide.ts | Switches from global window.loadPyodide to dynamic import from a versioned CDN URL. |
| src/repo-review-app/index.html | Removes the explicit Pyodide CDN <script> tag. |
| package.json | Moves pyodide into dependencies (needed at runtime for version resolution). |
| docs/webapp.md | Updates embedding/bundling notes to reflect automatic Pyodide loading. |
| docs/live-demo.md | Removes the explicit Pyodide CDN <script> tag from the live demo doc page. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onProgress?: (p: number, m?: string) => void, | ||
| ): Promise<PyodideInterface> { | ||
| const deps_str = deps.map((i) => `\"${i}\"`).join(", "); | ||
| const deps_str = deps.map((i) => `"${i}"`).join(", "); |
There was a problem hiding this comment.
deps_str is built by quoting each entry and later interpolated into a Python snippet for micropip.install([...]). This is unsafe/fragile if any dependency contains quotes/newlines (and allows Python code injection if deps can be influenced by an embedder). Prefer passing the JS array as data (e.g., via pyodide.globals or JSON encoding) rather than constructing Python source with string concatenation.
| // Version resolved from the npm package at build time; runtime files loaded from CDN | ||
| const PYODIDE_CDN_URL = `https://cdn.jsdelivr.net/pyodide/v${pyodidePackage.version}/full/pyodide.mjs`; |
There was a problem hiding this comment.
Pyodide is now always loaded from a hard-coded jsDelivr URL. This removes the ability for self-hosted/offline/CSP-restricted deployments to point Pyodide at an internal mirror or local files (previously they could provide loadPyodide themselves). Consider making the base URL configurable (e.g., parameter/env/global override) and defaulting to jsDelivr.
No longer required for users to load separately.