[HOLD for App onyx bump] Add getAllEntries function for Onyx. - #544
roryabraham wants to merge 3 commits into
Conversation
dangrous
left a comment
There was a problem hiding this comment.
Looks fine to me! I imagine it will be more fully tested as needed with that linked App PR. One NAB question but feel free to merge whenever.
| }, | ||
|
|
||
| getAllEntries() { | ||
| return Promise.resolve(Object.entries(store)); |
There was a problem hiding this comment.
NAB - why Object here instead of _ as above in getAllKeys()? Seems like the same effect either way?
There was a problem hiding this comment.
Generally we don't use underscore in TS files, and have lint rules to prevent it. but the lint config in Onyx is a bit different from E/App - most of our TS-specific lint rules are in E/App and should be moved to our ESLint plugin.
There was a problem hiding this comment.
ah okay got it. Should I go ahead and merge this or does it need to wait for the other related changes?
|
just nudging here - let me know if you want me to merge now! Not sure how it syncs up with the other PRs. |
|
I think with Onyx PRs it's a good practice to get the E/App PR fully tested, approved and ready to merged before merging the Onyx PR. That way the upgrade is more likely to go smoothly. |
|
yeah that makes sense. Will hold! |
|
The underlying feature was added separately w/ a different implementation, so this is no longer needed |
Details
Added specifically for importing and exporting full snapshots of Onyx data. Note that in the PR that enables this, I actually added a lint rule to restrict its usage.
Related Issues
https://expensify.slack.com/archives/C05LX9D6E07/p1714702412032799?thread_ts=1714656403.682529&cid=C05LX9D6E07
Automated Tests
This was straightforward, tested manually, and not intended for production use cases. So I didn't add automated tests.
Manual Tests
Follow manual testing steps from Expensify/App#41557
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop