- 
 
- 
  Notifications
 You must be signed in to change notification settings 
- Fork 57
BridgeJS: Improved @JS Protocol support - properties, Optional, enums #460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
BridgeJS: Improve SwiftHeapObject handling in import side
...rs / method return types in protocol
BridgeJS: Final refinings to enum support and revisiting tests
Thanks! The additional complication is a bit sad but it makes sense in terms of the performance. I think it might be worth researching the wasm-bindgen's approach here. It might give us some insight to come up with a better solution.
I know it seems like lots of additional intrinsics and new approaches, but I think most of it arises mostly from the fact that for most of the new types and features we've added since August, they support Swift -> JS side and not the other way around. So eventually we need to add JS -> Swift side support and not sure whether this could be further simplified (e.g. we could use side channels for everything for clarity but if we don't need to, then I think sentinel value seems better, but can't be used for all types) and most of what was needed to support properties in protocols and supporting all exportable types in methods was to actual bridge that gap in order to be able to import JS functionality in Swift Any wrapper.
Initially I overloaded existing intrinsics for ImportTS, as I think we could reuse the approach when we do JS -> Swift implementation for Optional etc, but went with dedicated methods to not created confusion (that JS -> Swift import is working), even though hard limitation is the check during lifting / lowering.
So let's consider whether this approach is fine for now 🙏🏻
I'll have a look in the meantime as wasm-bindgen 🫡
@kateinoigakukun regarding failing tests, I think I'm again missing something, I went through Contributing.md docs again and I always make sure to build examples using script in Utility and run tests for both BridgeJS and PackageToJS, but it seems that I can't force refresh for some of the build files, even running swift test --package-path ./Plugins/PackageToJS locally, but to no avail.
I tried to manually run tests for failing example package in ./Examples/Testing, according to README.md there, using:
swift package --disable-sandbox --swift-sdk wasm32-unknown-wasi js test, but it fails due to older macOS version requirement, so I don't think that's the issue 😅
Oh, I might have never run tests on macOS before.
3c2a981 to
 e7e1b79  
 Compare
 
 There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge after CI passed 👍
Introduction
This PR extends support for
@JS protocolby adding support for protocol properties,Optionaland additional support on types not previously supported, because of lack of theImportTSside.Overview
@JS protocoldeclarationsTesting
Added comprehensive tests in
Tests/BridgeJSRuntimeTests/ExportAPITests.swift:As I run into quite few issues along the way, I wanted to keep large test set, especially due to amount of new lifting / lowering methods added and use cases to test.
Documentation
Updated
Sources/JavaScriptKit/Documentation.docc/Articles/BridgeJS/Exporting-Swift/Exporting-Swift-Protocols.mdTechnical Details
I had few attempts at proper support for optionals and properties, but this is what I ended up with, was not able to simplify approach further while keeping it sensible, but any suggestions on improvements here are welcomed 🙏🏻
The implementation uses two different strategies for handling optional types in protocol properties, determined by WebAssembly ABI constraints:
Used when the WASM ABI can distinguish null from valid values using a sentinel:
Optional<Bool>: Uses-1for nil,0for false,1for trueOptional<CaseEnum>: Uses-1for nil,0+for enum casesOptional<AssociatedValueEnum>: Uses-1for nil,0+for case IDsOptional<SwiftHeapObject>: Uses0(null pointer) for nilUsed when WASM ABI cannot distinguish null from valid values (e.g., any valid value is possible):
Optional<Int>Optional<Float>Optional<Double>Optional<String>Optional<RawValueEnum>Side-channel uses global storage variables (
tmpRetOptionalInt,tmpRetString, etc.) that are set by the getter and read by the lifting code.Some new methods were required in BridgeJSIntrinsic, besides the one we used for most of the lifting / lowering:
bridgeJSLowerParameterWithPresence(): Converts optional to(isSome, value)tuple for protocol setters/parametersbridgeJSLiftReturnFromSideChannel(): Lifts optionals from side-channel storage for protocol property gettersFor sentinel-based lifting for types that support it, I added implementation for
bridgeJSLiftReturn(_ value: T):