Skip to content
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

[core][ios] Use key paths to refer to shared object properties #20610

Merged
merged 8 commits into from
Dec 27, 2022

Conversation

tsapeta
Copy link
Member

@tsapeta tsapeta commented Dec 26, 2022

Why

Implementing one of the ideas proposed in #20543

This introduces a syntactic sugar for the following:

Class(MySharedObject.self) {
  Property("foo") { mySharedObject in
    return mySharedObject.foo
  }
}

which could be replaced with the component that takes the key path

Class(MySharedObject.self) {
  Property("foo", \.foo)
}

Moreover, if the property is mutable (var property), it automatically sets the property setter too 🙂

How

  • Added two new versions of the Property definition that support Swift key paths (KeyPath for let properties and ReferenceWritableKeyPath for var properties in classes)
  • Added native unit tests

Test Plan

It's all green in native unit tests

@tsapeta tsapeta marked this pull request as ready for review December 26, 2022 23:26
"object.immutableKeyPathProperty"
])

// Returned value didn't change, it doesn't equal to `newValue`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would be even better if updating immutable property could have a compiler-time or build-time error. not sure whether it's feasible or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't seem possible, it's just a JS 🤷‍♂️ We can throw an error at runtime, but the current implementation is the same as in JS. Even Object.defineProperty(obj, "key", { writable: false }) still lets you do the assignment operation, but it's just a no-op and will return undefined when you read that property.

Also, I think it would be nice to add some attributes to the Property, such as writable, enumerable and readonly. The last one is not a part of the descriptor but it could change the behavior so that we throw an error when you try setting that property.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Object.defineProperty(obj, "key", { writable: false }) with no-op setter makes sense to me. thanks!

Base automatically changed from @tsapeta/ios/owned-properties to main December 27, 2022 10:50
@tsapeta tsapeta merged commit 80d7af3 into main Dec 27, 2022
@tsapeta tsapeta deleted the @tsapeta/ios/property-with-keypath branch December 27, 2022 13:20
@expo-bot
Copy link
Collaborator

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) 👇

⚠️ Suggestion: Missing links in changelog entries


I've added some suggestions below, you can just apply and commit them 😉


Generated by ExpoBot 🤖 against 449772c

@@ -12,6 +12,7 @@
- Trailing optional arguments can be skipped when calling native functions from JavaScript on iOS. ([#20234](https://github.com/expo/expo/pull/20234) by [@tsapeta](https://github.com/tsapeta))
- `Events` component can now be initialized with an array of event names (not only variadic arguments). ([#20590](https://github.com/expo/expo/pull/20590) by [@tsapeta](https://github.com/tsapeta))
- `Property` component can now take the native shared object instance as the first argument. ([#20608](https://github.com/expo/expo/pull/20608) by [@tsapeta](https://github.com/tsapeta))
- Added support for referencing to `Property`'s owner properties using Swift key paths.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Added support for referencing to `Property`'s owner properties using Swift key paths.
- Added support for referencing to `Property`'s owner properties using Swift key paths. ([#20610](https://github.com/expo/expo/pull/20610) by [@tsapeta](https://github.com/tsapeta))

@tsapeta tsapeta added the published Changes from the PR have been published to npm label Dec 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants