-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(core): queued storage (react-native) for logger #12798
Conversation
8677a78
to
61cd97f
Compare
912924f
to
38e0351
Compare
61cd97f
to
74d99c3
Compare
38e0351
to
e89ef1f
Compare
74d99c3
to
e77ccab
Compare
e89ef1f
to
f359840
Compare
e77ccab
to
d0b5c9a
Compare
|
||
const getQueuedItemKeys = async ( | ||
as: ReturnType<typeof loadAsyncStorage>, | ||
sortKeys: boolean = false |
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.
nit QQ
I think performance wise it's faster if we aren't manually sorting hence we default to false.
But I'm wondering if when this is used by CW, we'd always set this to true to make sure the older logs get a chance first
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.
Enable sortKeys
is less performant actually, it should only be used when there is a need, for example, get first N items from the queue. So made it an opt-in option here. Feel free to change though as this is an internal util.
const testBytesSize = 1; | ||
const mockKeys = [`${keyPrefix}_key1`, `${keyPrefix}_key2`]; | ||
const mockQueuedItems = [ | ||
{ |
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.
nit QQ
Is it possible the store might have other keys as well (without db prefix)
should we add a test wit those keys as well and check if
- those keys aren't getting updated/deleted.
- those keys getting changed don't have an impact on total currentBytesSize
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.
These items should've been covered with the test suites in this PR.
} from '../../../src/utils/queuedStorage/createQueuedStorage.native'; | ||
import { ItemToAdd, QueuedItem } from '../../../src/utils/queuedStorage/types'; | ||
import { getAddItemBytesSize } from '../../../src/utils/queuedStorage/getAddItemBytesSize'; | ||
|
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.
nit QQ
same comment
f359840
to
5596896
Compare
d0b5c9a
to
a160870
Compare
The base branch was changed.
a160870
to
e8d6212
Compare
Description of changes
Implementation of the queued storage for react-native.
Depends on #12791
NOTE: Please do NOT click the update branch button.
Issue #, if available
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.