Add native fetch() polyfill#188
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a native fetch() polyfill implemented in C++ on top of the existing UrlLib transports (matching the XMLHttpRequest approach), wires it behind a new CMake option, and extends the unit test suite to validate core fetch/Response behaviors.
Changes:
- Introduce
Polyfills/Fetchlibrary implementingfetch(input, init)and a minimalResponse/Headerssurface. - Wire
JSRUNTIMEHOST_POLYFILL_FETCH(default ON) and ensureUrlLibis fetched when either XHR or Fetch is enabled. - Add Mocha unit tests and a JSON asset to validate response properties and body readers (
text/arrayBuffer/json/blob) plus cloning and header behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UnitTests/Shared/Shared.cpp | Initializes the new Fetch polyfill in the UnitTests runtime. |
| Tests/UnitTests/Scripts/tests.ts | Adds a new describe("fetch") suite covering status/ok, statusText, body readers, headers, clone, init.method, and no-arg rejection. |
| Tests/UnitTests/CMakeLists.txt | Links the new Fetch target into the UnitTests executable; Assets are globbed/copied so sample.json is picked up. |
| Tests/UnitTests/Assets/sample.json | Adds deterministic JSON payload for the response.json() test. |
| Polyfills/Fetch/Source/Fetch.h | Declares internal Fetch initializer. |
| Polyfills/Fetch/Source/Fetch.cpp | Implements fetch(), Response-like object, and headers helpers over UrlLib. |
| Polyfills/Fetch/Readme.md | Documents supported surface area and known limitations. |
| Polyfills/Fetch/Include/Babylon/Polyfills/Fetch.h | Adds public API header for initializing the polyfill. |
| Polyfills/Fetch/CMakeLists.txt | Defines the Fetch library target and links against JsRuntime, arcana, and UrlLib. |
| Polyfills/CMakeLists.txt | Adds conditional inclusion of the Fetch subdirectory. |
| CMakeLists.txt | Adds JSRUNTIMEHOST_POLYFILL_FETCH option and updates UrlLib dependency gating. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed both review comments in ebe6a80:
This should turn the failing JSC/JSI/Android/Ubuntu/macOS/iOS builds green (they were all failing solely on the blob() test). |
|
CI is now fully green (23/23 ✅). Pushed four follow-up fixes after the initial submission, all surfaced by platform-specific CI:
Ready for re-review. |
Implements the WHATWG fetch() API as a native polyfill under Polyfills/Fetch/, mirroring the XMLHttpRequest polyfill layout. Like XMLHttpRequest, it is built on top of the platform-specific transports in UrlLib, so libcurl/WinHTTP/etc. behavior is identical between the two. fetch(input, init) returns a Promise that resolves to a Response-like object exposing ok/status/statusText/url/redirected/type/bodyUsed, a case-insensitive headers accessor (get/has/forEach), the body accessors text()/arrayBuffer()/json()/blob(), and clone(). Per the fetch spec, the promise only rejects on transport-level failures; a completed request with a non-2xx status (e.g. 404) still resolves with ok === false. The body is fully buffered before the promise resolves, so the body accessors may be called more than once (a deliberate lenient deviation from the spec's single-use semantics). Methods are limited to GET/POST and request bodies to strings, matching the underlying UrlLib transport. Wired behind the JSRUNTIMEHOST_POLYFILL_FETCH option (UrlLib is now fetched when either XMLHttpRequest or Fetch is enabled) and covered by new unit tests. Closes BabylonJS#98. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- blob() now detects the Blob polyfill via IsUndefined()/IsNull() instead of IsFunction(). Some JavaScriptCore/JSI builds classify constructor functions as typeof 'object', so IsFunction() incorrectly rejected even when the Blob polyfill was installed, failing the blob() test across all JSC/JSI CI jobs (matches the existing File polyfill workaround). - Add <cctype> for std::tolower/std::toupper used by ToLower/StatusText, which were relying on transitive includes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
arcana's task::then() captures the scheduler by reference and invokes it on the UrlLib worker thread when the request completes, which happens after the synchronous fetch() call has already returned. The previous stack-local JsRuntimeScheduler was therefore destroyed before the continuation ran, leaving arcana with a dangling reference. On Windows/Chakra this happened to survive, but on clang/libc++, JSC, JSI and Android it dereferenced freed memory and aborted with 'std::system_error: Invalid argument' inside JsRuntime::Dispatch (caught by ASAN as a stack-use-after-return). Heap-allocate the scheduler in a shared_ptr and co-own it from the continuation callable so it stays alive until the request completes, mirroring how XMLHttpRequest keeps its scheduler alive as a member. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Node-API-JSI declares Napi::Object::Set / Napi::Array::Set as non-const member functions, so calling Set() on 'const auto' instances failed to compile (C2663) on the *_JSI and Android targets while compiling fine under Chakra/V8. Declare the blob() 'parts' array and 'options' object as non-const, matching the other builder objects in this file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Android UnitTests app uses its own CMakeLists with an explicit polyfill link list, separate from the desktop Tests/UnitTests target. Without Fetch there, Shared.cpp failed to find <Babylon/Polyfills/Fetch.h> on Android_JSC and Android_V8. Add 'PRIVATE Fetch' to mirror the desktop test target. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1118799 to
fbe94db
Compare
…thod compares Address review feedback on the native fetch() polyfill: - Fix a hang: a throw inside the SendAsync continuation (e.g. from BuildResponse) was captured into the discarded task result and dropped, so the promise never settled and await fetch(...) hung. Split into two chained .then() continuations -- the first does the work and resolves, the second surfaces any error as a promise rejection. The scheduler is co-owned by the second continuation so it outlives the request. - Replace ToLower with a non-allocating EqualsIgnoreCase comparator used by both ParseMethod and FindHeader (removes per-header allocations). - Make ResponseData::body a plain std::vector<std::byte> instead of a shared_ptr; the struct is always held by shared_ptr, so the inner pointer only added an allocation and falsely implied clones get independent bodies. - Remove the unnecessary BuildResponse forward declaration. - Collapse the two fetch() catch blocks into a single catch (...) via the exception_ptr Error overload, removing the message-prefix asymmetry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follow-up to BabylonJS#188. UrlLib now exposes UrlRequest::StatusText() (the wire reason phrase where available, else a canonical code->text table), so: - Fetch: drop the private status-code->text table and read request.StatusText() into Response.statusText. - XMLHttpRequest: expose statusText (previously absent) for free. Adds fetch + XMLHttpRequest statusText tests (OK / Not Found). Bumps the UrlLib pin to BabylonJS/UrlLib 74985214, which adds UrlRequest::StatusText() (BabylonJS/UrlLib#30). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review, @bghgary — all comments are addressed in ac2019f, and the follow-ups you flagged are now resolved/tracked. Correctness fix (the blocker): the Cleanups: single
Node-API-JSC CI is green. Re-requested your review — PTAL. |
Follow-up to BabylonJS#188. UrlLib now exposes UrlRequest::StatusText() (the wire reason phrase where available, else a canonical code->text table), so: - Fetch: drop the private status-code->text table and read request.StatusText() into Response.statusText. - XMLHttpRequest: expose statusText (previously absent) for free. Adds fetch + XMLHttpRequest statusText tests (OK / Not Found). Bumps the UrlLib pin to BabylonJS/UrlLib 74985214, which adds UrlRequest::StatusText() (BabylonJS/UrlLib#30). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follow-up to BabylonJS#188. UrlLib now exposes UrlRequest::StatusText() (the wire reason phrase where available, else a canonical code->text table), so: - Fetch: drop the private status-code->text table and read request.StatusText() into Response.statusText. - XMLHttpRequest: expose statusText (previously absent) for free. Adds fetch + XMLHttpRequest statusText tests (OK / Not Found). Bumps the UrlLib pin to BabylonJS/UrlLib 74985214, which adds UrlRequest::StatusText() (BabylonJS/UrlLib#30). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implements the WHATWG etch() API as a native polyfill under
Polyfills/Fetch/, mirroring theXMLHttpRequestpolyfill layout. LikeXMLHttpRequest, it is built on the same platform-specificUrlLibtransports, so libcurl/WinHTTP/etc. behavior is identical between the two.Closes #98.
API
fetch(input, init)returns aPromisethat resolves to aResponse-like object exposing:ok,status,statusText,url,redirected,type,bodyUsedheaderswithget(name)/has(name)/forEach(cb)(case-insensitive)text(),arrayBuffer(),json(),blob()(each returns aPromise)clone()Behavior / deliberate limitations
ok === false.bodyUsedstaysfalse) — a lenient deviation from the spec's single-use semantics.GET/POSTonly and string request bodies only, matching the underlyingUrlLibtransport (same constraints asXMLHttpRequest).blob()requires theBlobpolyfill to be initialized.Wiring
JSRUNTIMEHOST_POLYFILL_FETCHoption (defaultON).UrlLibis now fetched when eitherXMLHttpRequestorFetchis enabled.describe("fetch")block (12 cases: status/ok, 404-resolves, statusText, text/arrayBuffer/json/blob, case-insensitive headers, clone, init method, no-arg rejection). A smallAssets/sample.jsonbacks the deterministicjson()test.Verification
Built the
Fetchtarget underwarnings_as_errorsand ran the full UnitTests suite on Win32/Chakra: 192 passing, including all 12 new fetch cases.Context
This is step 2 of the fetch native-polyfill pivot discussed in BabylonNative#1707 (reviewers asked fetch to follow the same native pattern as File/FileReader (#169) and AbortController). Once this lands, BabylonNative#1707 becomes a small bump that initializes
Polyfills::Fetch.