Add native JavaScript class bindings and optimized utilities#122
Add native JavaScript class bindings and optimized utilities#122scorsin-oai wants to merge 3 commits into
Conversation
Sensitive Files DetectedThis is an automated notice. A maintainer will review after import. |
clholgat
left a comment
There was a problem hiding this comment.
A couple of issues on the Base64 migration flagged during review (both would ship undetected given the current test matrix does not exercise web, and the TS specs aren't run in CI):
| } | ||
| return output.join(''); | ||
| } | ||
| import { decodeFromBase64, encodeToBase64 } from './Base64Native'; |
There was a problem hiding this comment.
This swaps the pure-JS Base64 implementation for the C++-backed Base64Native module, but there's no web polyglot fallback for it — unlike the Unicode migration in this same PR.
coreutils/web/ only contains UnicodeNative.ts, and web_register_native_module_id_overrides in coreutils/BUILD.bazel only maps UnicodeNative.js; there's no Base64Native entry. On web, ./Base64Native won't resolve at runtime, so encodeToBase64/decodeFromBase64 will be undefined and Base64.fromByteArray / toByteArray / byteLength will throw.
Could you add a coreutils/web/Base64Native.ts mirroring UnicodeNative.ts, plus a matching entry in the override map?
There was a problem hiding this comment.
yeah I was hoping that would all be part of my web PR which has this change, but I understand in the mean time this is breaking things on web.
There was a problem hiding this comment.
Realistically, it'll be at least a week before the web stuff lands at the earliest so we need a stop gap until then.
| if (validLen % 4 === 1) { | ||
| throw new Error('Invalid string. Length must be a valid Base64 length'); | ||
| } | ||
|
|
||
| const placeHoldersLen = validLen === b64.length ? (4 - (validLen % 4)) % 4 : 4 - (validLen % 4); | ||
| return ((validLen + placeHoldersLen) * 3) / 4 - placeHoldersLen; |
There was a problem hiding this comment.
byteLength no longer rejects malformed strings that contain = at a non-multiple-of-4 boundary, which the previous implementation threw on. The has-= branch uses 4 - (validLen % 4) without the % 4 wrap the no-padding branch has, and the only guard is validLen % 4 === 1, so:
byteLength('=')→-1(validLen=0 → placeHoldersLen=4 →(0+4)*3/4 - 4 = -1)byteLength('Zm9v=')→2(validLen=4 → placeHoldersLen=4 →6 - 4 = 2)
Both threw before this change. A decoded-size helper returning a negative/incorrect value is risky for any caller that uses it to size an allocation. Suggest rejecting = at a non-%4 boundary and adding tests for these cases — the suite currently only covers byteLength('Z').
There was a problem hiding this comment.
don't remember the exact specifics but I know I did that on purpose, byteLength() was more strict that our decoding code for no valid reason
There was a problem hiding this comment.
prior to this change: Base64 decode was lenient, it added missing padding before decoding, byteLength() was stricter, it did not accept some inputs that decode would accept. Now they accept unpadded inputs
There was a problem hiding this comment.
Agreed the unpadded leniency itself is good — 'Zg' → 1 and 'Zm9v' → 3 now is right, and matching decode there makes sense.
The thing I was flagging is narrower: for a few malformed inputs byteLength now returns a value that can't be a real byte length —
byteLength('=')→-1byteLength('==')→-1byteLength('Zm9v=')→2(would expect 3, or a reject)
The -1 is the one I'd want to avoid: a caller using byteLength to size a new Uint8Array(n) / buffer gets a negative length. Root cause is the =-present branch using 4 - (validLen % 4) without the % 4 wrap the no-= branch has, so a leading/misplaced = (where validLen % 4 === 0) gives placeHoldersLen = 4 and the formula goes negative.
Not a blocker and fine to defer — but might be worth clamping to 0, or rejecting = at a non-multiple-of-4 offset. Your call.
|
| Test Suite | Result |
|---|---|
| Valdi Smoke Tests | ❌ cancelled |
| API Surface Check | ✅ success |
| Snapshot Tests | ❌ cancelled |
| Linux: C++ Tests | ❌ cancelled |
| Linux: Build Compiler | ✅ success |
| Linux: Build & Export | ❌ cancelled |
| macOS: C++ & Platform Tests | ❌ failure |
Some tests failed. Please check the workflow logs for details.
🚀 Bazel remote cache is now enabled - future builds will be faster!
Workflow: Valdi CI
| JSValueRef V8JavaScriptContext::newNativeClass(const Ref<RefCountable>& /*classOpaque*/, | ||
| const JSClassDefinition& /*classDefinition*/, | ||
| JSExceptionTracker& exceptionTracker) { | ||
| exceptionTracker.onError("Native classes are not supported by V8"); |
There was a problem hiding this comment.
Not a blocker for this PR — flagging for the record since V8 is experimental and isn't wired into any shipping config yet.
This PR routes the Base64/Unicode module factories through newNativeClass, which throws here on V8. So on V8 those modules will now fail to load, where the previous pure-JS impls worked. It's invisible today because V8 is excluded from the test matrix and unused.
No action needed now — just noting it so that whenever V8 does get picked up, native-class support (or a V8 fallback for these factories) is a known prerequisite rather than a surprise. Might be worth a // TODO here pointing at that.
|
One V8 nit, but I'm pulling this in to start testing |
|
Native-class binder: required reference params crash on Following up on the review with one issue I think is worth fixing at the source before this lands. In if constexpr (kIsRequiredPrimitive<T>) {
if (isParameterNullOrUndefined(callContext, index)) {
callContext.getExceptionTracker().onError("Native class primitive argument cannot be null or undefined");
return T();
}
...
}But required reference params ( } else {
if (isParameterNullOrUndefined(callContext, index)) {
return T(); // null Ref<StaticString> / zero-initialized JSTypedArray, no exception set
}
...
}
So e.g. I'd suggest fixing this once in the binder rather than adding per-method null checks: have required reference types reject null the same way primitives already do. Genuinely optional params already go through the |
Description
This change adds complete support for bridging native classes to JS, which was something that has been missing for some time in Valdi.
Before this change, we only supported bridging native functions to JS. These functions can have attached states, as such we were able to go quite far without supporting actual classes. There were a few downsides
with that approach:
new MyNativeClass()where MyNativeClass was a real native class. We always had to make a TS class wrapper that then calls native with a call likethis.nativeInstance = nativeCreate().instanceofoperator to identify the class of a specific instance.This change also introduces some optimized utilities like a Base64 module backed by C++ code so that we don't need to do base64 in pure JS.
Type of Change
Testing
bazel test //...)Testing Details
Checklist
Related Issues
Additional Context