[TS migration] Migrate Onyx private methods to TS #523
Conversation
tgolen
changed the title
|
I'm placing this on HOLD right now in order for QA to be done on the latest version of Onyx without more un-QAed PRs from being merged. |
|
Absolutely! The PR is still WIP, so no need to worry that is going to be merged too soon |
|
Sorry @aldo-expensify, I miss-clicked. @fabioh8010 is still reviewing this PR, once it's approved I'll mark it as ready for review 😄 |
|
@aldo-expensify All yours! It was already tested in NewDot by me and @fabioh8010 😄 |
| type DefaultConnectOptions<TKey extends OnyxKey> = { | ||
| key: TKey; | ||
| callback?: DefaultConnectCallback<TKey>; | ||
| waitForCollectionCallback?: false; | ||
| }; | ||
|
|
||
| /** Represents the callback function used in `Onyx.connect()` method with a collection key. */ | ||
| type CollectionConnectOptions<TKey extends OnyxKey> = { | ||
| key: TKey extends CollectionKeyBase ? TKey : never; | ||
| callback?: CollectionConnectCallback<TKey>; | ||
| waitForCollectionCallback: true; | ||
| }; |
There was a problem hiding this comment.
Last comment/question: Wondering why these types were not defined like this:
| type DefaultConnectOptions<TKey extends OnyxKey> = { | |
| key: TKey; | |
| callback?: DefaultConnectCallback<TKey>; | |
| waitForCollectionCallback?: false; | |
| }; | |
| /** Represents the callback function used in `Onyx.connect()` method with a collection key. */ | |
| type CollectionConnectOptions<TKey extends OnyxKey> = { | |
| key: TKey extends CollectionKeyBase ? TKey : never; | |
| callback?: CollectionConnectCallback<TKey>; | |
| waitForCollectionCallback: true; | |
| }; | |
| type DefaultConnectOptions<TKey extends Key> = { | |
| key: TKey; | |
| callback?: DefaultConnectCallback<TKey>; | |
| waitForCollectionCallback?: false; | |
| }; | |
| /** Represents the callback function used in `Onyx.connect()` method with a collection key. */ | |
| type CollectionConnectOptions<TKey extends CollectionKeyBase> = { | |
| key: TKey; | |
| callback?: CollectionConnectCallback<TKey>; | |
| waitForCollectionCallback: true; | |
| }; |
DefaultConnectOptions<TKey extends OnyxKey> feels a bit wrong since OnyxKey can be a collection key, then the waitForCollectionCallback: false would be wrong for such key. Also CollectionConnectOptions<TKey extends CollectionKeyBase> allows you to do key: TKey instead of the more complex TKey extends CollectionKeyBase ? TKey : never;.
Do you see any downside of specifying these types like this?
There was a problem hiding this comment.
I was like, the type looks weird indeed but it got me thinking that it was defined like this for a reason 😅
I checked in NewDot, this type works until you use it with a key (not a collection key) and add waitForCollectionCallback: true:
let allCards: OnyxValues[typeof ONYXKEYS.CARD_LIST] = {};
Onyx.connect({
key: ONYXKEYS.CARD_LIST,
callback: (val) => {
if (!val || Object.keys(val).length === 0) {
return;
}
allCards = val;
},
waitForCollectionCallback: true,
});It makes val: OnyxCollection<CardList> while it should be val: OnyxEntry<CardList>. With the old type errors properly: Type 'true' is not assignable to type 'false'
There was a problem hiding this comment.
Oh, interesting, wouldn't that mean that ONYXKEYS.CARD_LIST is a valid CollectionKeyBase when it shouldn't be?
There was a problem hiding this comment.
In your example, if things worked properly, I would have expected one of these two errors:
key: ONYXKEYS.CARD_LISTis wrong becausekeymust be aCollectionKeyBaseand is not, orwaitForCollectionCallback: truehas an error becauseONYXKEYS.CARD_LISTis aKeyand therefore,waitForCollectionCallbackmust befalseor undefined
There was a problem hiding this comment.
Oh, interesting, wouldn't that mean that ONYXKEYS.CARD_LIST is a valid CollectionKeyBase when it shouldn't be?
I checked it in ONYXKEYS.ts in NewDot and ONYXKEYS.CARD_LIST is not a valid CollectionKeyBase:
const cardList1: OnyxCollectionKey = 'cardList'; // errors
const cardList2: OnyxValueKey = 'cardList'; // works fineFor context, OnyxCollectionKey and OnyxValueKey are used in src/types/modules/react-native-onyx.d.ts to augment onyx keys:
declare module 'react-native-onyx' {
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
interface CustomTypeOptions {
keys: OnyxValueKey | OnyxFormKey | OnyxFormDraftKey;
collectionKeys: OnyxCollectionKey;
values: OnyxValues;
}
}This way keys, and collectionKeys are no longer string but instead are union of literal strings
There was a problem hiding this comment.
In your example, if things worked properly, I would have expected one of these two errors:
key: ONYXKEYS.CARD_LIST is wrong because key must be a CollectionKeyBase and is not, or
waitForCollectionCallback: true has an error because ONYXKEYS.CARD_LIST is a Key and therefore, waitForCollectionCallback must be false or undefined
As you already noted, all Key, OnyxKey and CollectionKeyBase are of type string in react-native-onyx. It's later augmented in the end-user repo like NewDot, that's why it is so tricky to understand.
While this seems like a great idea, it comes with a problem. It does work until all Key, OnyxKey and CollectionKeyBase are of type string, but once we augment these values through src/types/modules/react-native-onyx.d.ts file, ConnectOptions type breaks:
type Key = 'account' | 'isSidebarLoaded' ;
type CollectionKeyBase = 'report_';
type OnyxKey = Key | CollectionKey;
type DefaultConnectOptions<TKey extends Key> = {
key: TKey;
callback?: DefaultConnectCallback<TKey>;
waitForCollectionCallback?: false;
};
type CollectionConnectOptions<TKey extends CollectionKeyBase> = {
key: TKey;
callback?: CollectionConnectCallback<TKey>;
waitForCollectionCallback?: true;
};
type ConnectOptions<TKey extends OnyxKey> = CollectionConnectOptions<TKey> | DefaultConnectOptions<TKey>;
// Type 'TKey' does not satisfy the constraint 'Key'.
// Type 'OnyxKey' is not assignable to type 'Key'.That is why TS struggles to show an error here - when defining the type in onyx it thinks it is all right to use Key, OnyxKey and CollectionKeyBase interchangeably, but once these types are detached from each other (augmented in NewDot) TS doesn't throw an error when it should.
There was a problem hiding this comment.
I think we should leave ConnectOptions as is, @fabioh8010 maybe you have some idea as you created this type?
There was a problem hiding this comment.
I see, well, thanks a lot for trying and following up with this. Meanwhile, I'll merge since the PR leaves us in a better place anyway and I don't want to keep blocking here. If you come up with some improvements later, just lets do them in a follow up PR.
There was a problem hiding this comment.
This is the first time I see type "augmentation" in typescript, pretty cool feature!
There was a problem hiding this comment.
I think is better to keep the way it is for now, this was the best solution we found at that time to add this type safety around waitForCollectionCallback and collection keys.
|
🚀Published to npm in v2.0.28 |
Details
This PR migrates "private" Onyx.js methods that we use in NewDot, "private" methods were moved to
OnyxUtils.jsin this PR.Related Issues
Part of Expensify/App#34344
Automated Tests
N/A
Manual Tests
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.webm
Android: mWeb Chrome
android-web.webm
iOS: Native
ios.mp4
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov