iOS CompletedState includes a read-only data controller; omit set from core
#786
Conversation
| public func makeReadOnly() -> ReadOnlyDataController? { | ||
| value.invokeMethod("makeReadOnly", withArguments: []) | ||
| .map { .createInstance(value: $0) } | ||
| } |
There was a problem hiding this comment.
Question for reviewers: is this something that we actually want users to be able to access on core/ios/android/etc? It strikes me as a helper function that's been exposed to users.
There was a problem hiding this comment.
I could see a case where a user might want to access it in a custom plugin but might be rare
There was a problem hiding this comment.
Yeah. My preference is private/internal if possible. The more we expose, the larger our public api, and the higher the surface-area for breaking changes. There doesn't really appear to be a strong benefit to exposing this right now so I'm in favour of not to limit public api.
There was a problem hiding this comment.
Hm, I revise my statement. It looks like it must be public in the web api because of the way we're using it. So I'll leave the iOS API so that there's parity.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## player-1-dot-zero #786 +/- ##
====================================================
Coverage ? 85.90%
====================================================
Files ? 507
Lines ? 22891
Branches ? 2656
====================================================
Hits ? 19664
Misses ? 2898
Partials ? 329 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /** Wrapper for the Data Controller Class that prevents writes */ | ||
| export class ReadOnlyDataController | ||
| implements DataModelWithParser<DataModelOptions> | ||
| implements Pick<DataModelWithParser<DataModelOptions>, "get" | "delete"> |
There was a problem hiding this comment.
If the data controller is going to be read only we shouldn't support delete
What
setanddeletefrom coreReadOnlyDataController. They previously just logged an error and did nothing. Omittingsetanddeleteis a cleaner API. This is outside the scope of the original issue but was discussed with @KetanReddyReadOnlyDataControlleron iOS. This wraps the core version of the same. ACompletedStateon iOS will now include this controller.makeReadOnly()to the iOSDataController. It's not used on iOS, however.Why
#219
We should have parity across all platforms. This updates ios to match the same public API as the core for the Read-Only Data Controller. The read-only version of the data controller was introduced to allow users to access data / evaluate bindings after a flow has ended.
Change Type (required)
Indicate the type of change your pull request is:
patchminormajorN/A(this is part of the major release)Does your PR have any documentation updates?