[core] Add nativeRefType to SharedRef#31776
Conversation
|
aleqsio
left a comment
There was a problem hiding this comment.
Like the approach with essentially tagging the SharedRef.
One thought:
Would it maybe be possible to compute the tag based on the underlying native class?
If you create SharedRef(image), the nativeRefType would be UIImage on iOS and Drawable on Android, with the TS side code having an explicit check for both options.
|
|
||
| Class(SharedString.self) { | ||
| Constructor { | ||
| SharedString("string") |
There was a problem hiding this comment.
Do we need this argument if we already override nativeRefType? Seems duplicative.
There was a problem hiding this comment.
The string here is a value of the reference, not the nativeRefType.
| to native instances among different independent libraries. | ||
| */ | ||
| open class SharedRef<RefType>: SharedObject { | ||
| open class SharedRef<RefType>: SharedObject, AnySharedRef { |
There was a problem hiding this comment.
Would it make sense to have a constructor like SharedRef("image", drawable) so that you can create refs without inheriting from SharedRef?
There was a problem hiding this comment.
I don't think providing the nativeRefType would be common. There aren't many objects that make sense to move between packages, so I don't think adding such a constructor would be necessary. Also, that gives you two different ways of providing the nativeRefType.
aafe118 to
8d1f419
Compare
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
|
The Pull Request introduced fingerprint changes against the base commit: e10d05f Fingerprint diff[
{
"op": "changed",
"source": {
"type": "dir",
"filePath": "../../packages/expo-image-manipulator/android",
"reasons": [
"expoAutolinkingAndroid"
],
"hash": "1a775d921077fd057d2a36b59ae17f85ed186b46"
}
},
{
"op": "changed",
"source": {
"type": "dir",
"filePath": "../../packages/expo-image-manipulator/ios",
"reasons": [
"expoAutolinkingIos"
],
"hash": "aa9a2923eace2f8861b663b5bc6f6b8baffe1541"
}
},
{
"op": "changed",
"source": {
"type": "dir",
"filePath": "../../packages/expo-image/android",
"reasons": [
"expoAutolinkingAndroid"
],
"hash": "44fd530d42c77f566613284bee8d98b72955afc4"
}
},
{
"op": "changed",
"source": {
"type": "dir",
"filePath": "../../packages/expo-image/ios",
"reasons": [
"expoAutolinkingIos"
],
"hash": "819f8520268619eb96ecb63bba4f545803cff264"
}
},
{
"op": "changed",
"source": {
"type": "dir",
"filePath": "../../packages/expo-modules-core",
"reasons": [
"expoAutolinkingIos",
"expoAutolinkingAndroid"
],
"hash": "2fb8d1d120cc357d35c62fdfa99fdf52888c250b"
}
}
]Generated by PR labeler 🤖 |
|
Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines. I've found some issues in your pull request that should be addressed (click on them for more details) 👇
|
Why
Adds
nativeRefTypetoSharedRef.SharedRef's can now be checked on the JS side to ensure the native type is correct when passed across packages. For example, if you pass a SharedRef holding an image, the module should verify that it has not received a different type.
How
Added a new property called
nativeRefType. In the future, we will probably add a field containing information about the native class.Test Plan