Add File / FileReader polyfill#169
Open
bkaradzic-microsoft wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves File / FileReader support from a BabylonNative Playground-only JS shim into JsRuntimeHost as a native C++ polyfill, so all JsRuntimeHost consumers get the WHATWG-style APIs alongside existing polyfills (Blob, URL, XHR, etc.).
Changes:
- Add a new
Polyfills/File/C++ target implementingFileandFileReader(including event handler slots + EventTarget-style listeners). - Wire the polyfill into the build via a new
JSRUNTIMEHOST_POLYFILL_FILECMake option (ON by default) and link it into unit tests. - Add new Mocha unit tests covering
FileandFileReaderbehaviors (construction, reads, events, abort, and a JSC regression aroundObject.prototypepollution).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Adds JSRUNTIMEHOST_POLYFILL_FILE option (ON by default). |
| Polyfills/CMakeLists.txt | Conditionally includes the new File polyfill subdirectory. |
| Polyfills/File/CMakeLists.txt | Defines the File polyfill library target and its sources. |
| Polyfills/File/Include/Babylon/Polyfills/File.h | Public API entrypoint for initializing the polyfill. |
| Polyfills/File/Readme.md | Documents behavior and prerequisites (Blob must be initialized first). |
| Polyfills/File/Source/File.h | Declares the internal File ObjectWrap implementation. |
| Polyfills/File/Source/File.cpp | Implements File over the existing Blob polyfill and initializes FileReader. |
| Polyfills/File/Source/FileReader.h | Declares the internal FileReader ObjectWrap implementation. |
| Polyfills/File/Source/FileReader.cpp | Implements async reads, base64 DataURL generation, events, and abort logic. |
| Tests/UnitTests/CMakeLists.txt | Links the new File target into the UnitTests executable. |
| Tests/UnitTests/Shared/Shared.cpp | Initializes the File polyfill in the unit test JS environment. |
| Tests/UnitTests/Scripts/tests.ts | Adds new Mocha tests for File and FileReader, including a JSC regression canary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6f8a827 to
67f9f5c
Compare
5d68379 to
bf6db1c
Compare
Implements the WHATWG File and FileReader web APIs as a native
polyfill, alongside the existing Blob polyfill that File delegates to
for byte storage.
* `Polyfills/File/` -- new polyfill target, gated on a new
`JSRUNTIMEHOST_POLYFILL_FILE` CMake option (ON by default).
* `Tests/UnitTests/Scripts/tests.ts` -- 28 new Mocha tests
(13 File + 15 FileReader), including a regression canary for
Object.prototype pollution by `FileReader.EMPTY/LOADING/DONE`.
Notable implementation details
------------------------------
* FileReader registers its `EMPTY/LOADING/DONE` constants via
`StaticValue` and `InstanceValue` descriptors inside the
`DefineClass` property list. On JavaScriptCore the napi shim's
`ConstructorInfo::Create` defaults the constructor's `.prototype`
to `Object.prototype`, so setting these via
`func.Get("prototype").Set(...)` would pollute every plain object
in the runtime.
* The Blob-dependency guard in `File::Initialize` uses `IsUndefined()`
rather than `IsFunction()`. Some JSC builds (notably
`libjavascriptcoregtk` on Linux) classify constructors created via
`JSObjectMakeConstructor` as `typeof 'object'`, not `'function'`,
so `napi_typeof` returns `napi_object` for them and an
`IsFunction()` guard would silently early-return on those engines.
`IsUndefined()` matches the guard used by Blob and works on every
engine we ship.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bf6db1c to
e0ab15b
Compare
…on Chakra ThrowAsJavaScriptException avoids the Chakra heap-corruption on throw-from-constructor, but JSI's napi shim implements `Error::ThrowAsJavaScriptException` as a no-op that only stores the error in `env->last_exception` without raising it. The script then continues normally with an incompletely-initialized File wrapper, the `expect(...).to.throw()` matcher sees no JS error, and the unhandled exception is later reported through AppRuntime's UnhandledExceptionHandler, failing the test harness exit code. Use the JRH-wide convention `throw Napi::TypeError::New(env, msg)` (matches Blob, XHR, URL, AbortSignal, TextDecoder, FileReader). On engines whose Node-API shim properly translates a C++ throw into a JS exception at the ObjectWrap construction boundary (V8 / JSC / JSI), this propagates as expected. Chakra cannot handle throwing from a constructor — there are five pre-existing TODOs in tests.ts noting this same limitation for URL parse() error cases. Apply the same workaround here: keep the WebIDL-conformant throw in C++, but comment out the test that exercises it (it would corrupt the Chakra heap on every CI run). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9a3e2f3 to
fa84e76
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The File polyfill in BabylonNative currently lives as a JS shim
(
Apps/Playground/Scripts/file_polyfill.js, ~308 lines) that isLoadScript-ed from the Playground'sAppContext(seeBabylonJS/BabylonNative#1706). Per @bghgary's review on that PR, it
should live in JsRuntimeHost alongside Blob, URL, WebSocket,
XMLHttpRequest, TextDecoder, AbortController, etc., so every
JsRuntimeHost consumer -- not only the BabylonNative Playground --
gets
FileandFileReader.This PR adds a native C++
Polyfills/File/target that implementsthe WHATWG
FileandFileReaderweb APIs, and 28 new Mocha unittests (13 File + 15 FileReader) under
Tests/UnitTests/Scripts/tests.ts.Surface area
File(parts, name, options)constructor.size,type,name,lastModified.arrayBuffer(),text(),bytes().FileReaderwith state constantsEMPTY/LOADING/DONE(both on the constructor and on instances, per WHATWG).
FileReaderevents:loadstart,load,loadend,progress,error,abortvia bothonXhandler slots andaddEventListener/removeEventListener.readAsText/readAsArrayBuffer/readAsDataURL/readAsBinaryString/abort.Implementation notes
Blobpolyfill viaenv.Global().Get("Blob")so we reuse Blob'sBlobParthandling(
ArrayBuffer, typed array, string, Blob).readAsDataURLis an inlinedRFC 4648 implementation -- JsRuntimeHost has no base-n dependency.
JSRUNTIMEHOST_POLYFILL_FILECMake option is on by defaultand gates building the target.
Notable JSC-specific care
Two pieces of the implementation are shaped specifically to work
correctly on JavaScriptCore:
FileReader.EMPTY/LOADING/DONEare registered viaStaticValueandInstanceValuedescriptors inside theDefineClassproperty list, not viafunc.Get("prototype").Set(...)after class creation. On JSC thenapi shim's
ConstructorInfo::Createdefaults the constructor's.prototypetoObject.prototype, so the latter pattern wouldpollute
Object.prototypewithEMPTY/LOADING/DONEkeys andbreak
for..inover plain objects throughout the runtime. TheTests/UnitTests/Scripts/tests.tsregression test"does not pollute Object.prototype with EMPTY/LOADING/DONE"pinsthis behaviour.
The Blob-dependency guard in
File::InitializeusesIsUndefined()rather thanIsFunction(). Some JSC builds(notably
libjavascriptcoregtk-4.1on Linux) classify constructorscreated via
JSObjectMakeConstructorastypeof 'object', not'function', sonapi_typeofreturnsnapi_objectfor themand an
IsFunction()guard would silently early-return on thoseengines.
IsUndefined()matches the guard used by Blob and workson V8, JSI, Chakra, iOS-JSC, Android-JSC, and Linux JSC.
Testing
Locally:
UnitTests.exeon Win32 V8 Release passes 167/167 tests(139 pre-existing + 28 new), including the Object.prototype-pollution
regression canary.
End-to-end with BabylonNative: with this PR pinned in
BabylonJS/BabylonNative#1706, BabylonNative CI passes 26/26 jobs(all platforms including Linux Clang/GCC JSC), and the 19 GLTF/OBJ
playground tests that previously depended on the JS shim are
re-enabled and passing.
After this merges
BabylonJS/BabylonNative#1706re-pinsCMakeLists.txt'sGIT_REPOSITORY/GIT_TAGto the merged SHA and then merges.