Skip to content

feat: add config option to disable sync events for some keys#124

Merged
marcaaron merged 3 commits into
Expensify:mainfrom
mdneyazahmad:feat/disable-sync-event
Apr 25, 2022
Merged

feat: add config option to disable sync events for some keys#124
marcaaron merged 3 commits into
Expensify:mainfrom
mdneyazahmad:feat/disable-sync-event

Conversation

@mdneyazahmad

@mdneyazahmad mdneyazahmad commented Apr 12, 2022

Copy link
Copy Markdown
Contributor

Details

When user opens multiple tabs with different reports. We do not want some data in onyx to sync across tabs. This pr will add a feature that we can configure keys for which we do not want to subscribe to data change.

Related Issues

#127

Automated Tests

We'd have a hard time writing an automated test here as we need the storage event to be mocked somehow.

Linked PRs

Expensify/App#8759

@github-actions

github-actions Bot commented Apr 12, 2022

Copy link
Copy Markdown
Contributor

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@mdneyazahmad

Copy link
Copy Markdown
Contributor Author

recheck

@mdneyazahmad

Copy link
Copy Markdown
Contributor Author

@marcaaron Based on your suggestion Expensify/App#8227 (comment) I drafted this pr. I am new in this repository. Could you please confirm, we need unit test for this pr?

@marcaaron marcaaron left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This LGTM. We'd have a hard time writing an automated test here as we need the storage event to be mocked somehow. Manual tests should be fine.

Comment thread lib/storage/WebStorage.js Outdated
Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>
@mdneyazahmad mdneyazahmad marked this pull request as ready for review April 17, 2022 11:08
@mdneyazahmad mdneyazahmad requested a review from a team as a code owner April 17, 2022 11:08
@melvin-bot melvin-bot Bot requested review from nkuoch and removed request for a team April 17, 2022 11:09
@mdneyazahmad

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@marcaaron

Copy link
Copy Markdown
Contributor

@mdneyazahmad can you please do the following...

  • Fix lint issues
  • Provide a short summary of the changes in Details
  • Link the correct issue in the Related Issues section
  • Give an explanation about why no automated tests were added in the Automated Tests
  • Create a new PR that will update the version of react-native-onyx in Expensify/App package.json and add it to the Linked PRs section

Thanks!

@mdneyazahmad

Copy link
Copy Markdown
Contributor Author

@marcaaron updated

@marcaaron marcaaron merged commit 7ab6aed into Expensify:main Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants