-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add snapshot functionality #552
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
lib/Onyx.ts
Outdated
if (!snapshotValue) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snapshot may not be read yet and thus not present in cache.
One option is to ignore it like this. Other option is to read all keys, filter out only snapshot key, then get value for them and compare.
Not sure which is preferred way @luacmartins
return clearPromise.then(() => Promise.all(promises.map((p) => p()))).then(() => undefined); | ||
return clearPromise | ||
.then(() => Promise.all(promises.map((p) => p()))) | ||
.then(() => updateSnapshots(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins currently snapshots are updated only after update
. Only this was specified in design doc. This means that if data are changed for example through merge
snapshots will not be updated. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think that's fine since we only want snapshot data to be updated from Pusher updates, which calls Onyx.update
. Are there other edge cases where the API calls merge
instead of update
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any edge case right now but I think it can be confusing if one forgets about this fact. After all it's open-source library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good code @jnowakow, however in general I see that there might be potential problems with the amount of iterations made.
So I think @luacmartins that this will have to monitored and measured, because it feels like the amount of keys in snapshot might grow very quickly and the we will be doing thousands of iterations per 1 snapshot update.
Side note I prefer how in this sibling PR: https://github.com/Expensify/react-native-onyx/pull/555/files we allow for different names for the SNAPSHOT and data
key names. Feels much more open and less arbitrary that you can configure the snapshot collection name.
I would move forward with this other PR
Reviewer Checklist
Screenshots/VideosAndroid: NativeN/A Android: mWeb ChromeScreen.Recording.2024-05-31.at.7.01.01.PM.moviOS: NativeN/A iOS: mWeb SafariScreen.Recording.2024-05-31.at.7.00.01.PM.movMacOS: Chrome / SafariScreen.Recording.2024-05-31.at.6.53.57.PM.movMacOS: DesktopScreen.Recording.2024-05-31.at.6.56.21.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this yet, but code changes LGTM. Waiting on @allroundexperts review and tests to test this more in depth
Yep, I'm on it! |
How you getting on @allroundexperts? |
@trjExpensify I was unable to verify the fix. For me, the table values do not get updated. |
Screen.Recording.2024-05-28.at.7.36.10.PM.mov |
@allroundexperts problem lies in App. Two days ago this PR was merged. It wraps all components in Search table in For testing purposes you can modify const TotalCell = memo(({showTooltip, amount, currency, isLargeScreenWidth}: CurrencyCellProps) => {
const styles = useThemeStyles();
return (
<TextWithTooltip
shouldShowTooltip={!!showTooltip}
text={CurrencyUtils.convertToDisplayString(amount, currency)}
style={[styles.optionDisplayName, styles.textNewKansasNormal, styles.pre, styles.justifyContentCenter, isLargeScreenWidth ? undefined : styles.textAlignRight]}
/>
);
}); It's defined in Go-to solution is to modify functions passed to |
Nice find @jnowakow! Yea, let's update the prop comparison function to handle these updates too! |
@luacmartins sure thing! I'll work on that. |
@allroundexperts were you able to test following @jnowakow's instructions here? |
Not really. Isn't this on hold? |
I think just a merge hold. We can still continue development. What part of the instructions didn't work? |
Oh, I didn't continue the testing after hold was applied. Will re-test and get back today. |
Just tested this again. The new search page seems to be broken now 😞 Screen.Recording.2024-05-31.at.6.16.21.PM.mov |
@allroundexperts yea, we messed up the API a bit. To temporarily fix it you can change this value to |
@luacmartins It works now. But on Android, I'm getting the following crash: Screen.Recording.2024-05-31.at.7.03.41.PM.mov |
@allroundexperts that seems unrelated to this PR. Can we focus testing this Onyx changes in Web first? We should be able to see the live updates in Search when they are edited. |
I can confirm that for web, it is working fine. |
Working on the checklist now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small comment. Looks good otherwise!
The holding PR was merged, we can remove the hold here |
🚀Published to npm in v2.0.49 |
Details
This PR adds snapshot functionality described in design doc. It adds special support for
SNAPSHOT
key. Objects inside itsdata
field will be updated with every change made throughupdate
function.Related Issues
Expensify/App#41626
Automated Tests
Manual Tests
Author Checklist
### Related Issues
section aboveTests
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov