Add Emscripten/Pyodide support#1022
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Merging this PR will not alter performance
Comparing 
Footnotes
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 20 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/download-pyodide">
<violation number="1" location="scripts/download-pyodide:18">
P2: `pyodide_js.loadPackage()` is an async function returning a Promise, but it is called without `await` or any synchronization. The Python process may exit before packages finish loading, causing nondeterministic environment setup.</violation>
</file>
<file name="tests/httpx2/emscripten/conftest.py">
<violation number="1" location="tests/httpx2/emscripten/conftest.py:65">
P2: Worker fixture lookup is based on an unsupported naming convention, so worker-based emscripten tests will fail to find a fixture.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| [tool.coverage.paths] | ||
| source = [ | ||
| "src/httpx2/httpx2", | ||
| "*/site-packages/httpx2", | ||
| ] |
There was a problem hiding this comment.
How does this interact with source_pkgs above?
There was a problem hiding this comment.
I think it is orthogonal: [tool.coverage.run] indicates what files should get coverage information initially, [tool.coverage.paths] controls how coverage combine remaps paths. I think when you run the native tests, it uses an editable install so the file paths point into the source tree. On the other hand, in Pyodide it has to install the wheel into site-packages, and this tells it to remap the path back to the source tree path.
| cert_pem_file: str, | ||
| cert_private_key_file: str, |
There was a problem hiding this comment.
Should we just add those to the above server fixture, and have a single one?
|
|
||
| # For testing on emscripten, we require cross origin isolation headers | ||
| # to be set or else browsers won't be able to read from us from javascript | ||
| async def hello_world_emscripten(scope: Scope, receive: Receive, send: Send) -> None: # pragma: nocover for emscripten |
There was a problem hiding this comment.
Add a new custom pragma in exclude_also in coverage report called pragma: emscripten please.
There was a problem hiding this comment.
Isn't # pragma: nocover emscripten clearer than # pragma: emscripten?
| # For testing on emscripten, it is useful to be able to | ||
| # get the wheel package so that we can install it e.g. | ||
| # on web-workers | ||
| async def wheel_download(scope: Scope, receive: Receive, send: Send) -> None: # pragma: nocover for emscripten | ||
| wheel_file = sorted(Path("dist").glob("httpx2-*.whl"))[0] | ||
| await send( | ||
| { | ||
| "type": "http.response.start", | ||
| "status": 200, | ||
| "headers": [ | ||
| [b"content-type", b"application/x-wheel"], | ||
| [b"access-control-allow-origin", b"*"], | ||
| [ | ||
| b"access-control-allow-methods", | ||
| b"PUT, GET, HEAD, POST, DELETE, OPTIONS", | ||
| ], | ||
| [b"Access-Control-Allow-Headers", b"*"], | ||
| ], | ||
| } | ||
| ) | ||
| wheel_bytes = wheel_file.read_bytes() | ||
| await send({"type": "http.response.body", "body": wheel_bytes}) |
There was a problem hiding this comment.
This seems a bit weird. The endpoints here are used by the tests.
Just pointing out.
| IGNORE_ARGS="--omit=src/httpx2/httpx2/_transports/jsfetch.py,tests/httpx2/emscripten/*" | ||
| fi | ||
|
|
||
| uv run coverage report ${IGNORE_ARGS} --show-missing --skip-covered --fail-under=100 |
There was a problem hiding this comment.
I'm inclined to not have this machinery here, and not care about coverage on jsfetch.py. 🤔
There was a problem hiding this comment.
For what it's worth, the coverage information has been quite useful when editing jsfetch.py, the uncovered lines are often buggy.
There was a problem hiding this comment.
I'm a coverage purist. I understand this.
Maybe we can have it self contained? e.g. needs to be 100% on the jsfetch and test file. Instead of combining I mean.
There was a problem hiding this comment.
Well we'll need to combine even if we just want to check jsfetch.py because each call to @run_in_pyodide generates it's own separate coverage file.
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/overrides/pyodide.html">
<violation number="1" location="docs/overrides/pyodide.html:77">
P2: Development-mode detection uses overly broad origin prefix matching instead of exact hostname comparison</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/overrides/pyodide.html">
<violation number="1" location="docs/overrides/pyodide.html:77">
P2: Development-mode detection uses overly broad origin prefix matching instead of exact hostname comparison</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
3 participants
Checklist