-
Notifications
You must be signed in to change notification settings - Fork 13
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
[PRD-573] Destroy local storage #1552
Conversation
7bd112b
to
10df942
Compare
We introduce a `destroy` method in the storage interface, that will be called after a `client.reset()`. We implement this `destroy` in the localStorage platform, but it should be implemented accordingly to deal with other storage context, such as with indexedDB for workers or async-storage for react-native
10df942
to
800005e
Compare
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.
👍
return Promise.all( | ||
Object.values(this.pouches).map(pouch => pouch.destroy()) | ||
) | ||
await allSettled(Object.values(this.pouches).map(pouch => pouch.destroy())) |
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.
There's nothing left to return. 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.
Not really, I changed it
* when an empty iterable is passed), with an array of objects that describe the | ||
* outcome of each promise. | ||
* This implementation is useful for env with no support of the native Promise.allSettled, | ||
* typically react-native 0.66 |
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.
According to https://github.com/facebook/react-native/releases/tag/v0.70.6, we can now safely use Promise.allSettled on the flagship app.
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.
But I assume we might have users using out-of-date flagship in RN 0.66?
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.
Maybe, not prioritary to do anyway, but I think it starts to be safe to change
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.
This won't be problematic as this is packaged in the build, so old flagship version will use the old code with custom method and new versions will use the new one. No retro-compatibility needed.
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.
But there is no priority at all as those methods are equivalent
94a80eb
to
388d8c0
Compare
Rather than destroying the whole localstorage, which can be too aggressive, we rather destroy the items related to pouch link
388d8c0
to
14404ad
Compare
We introduce a
destroy
method in the storage interface, that willbe called after a
client.reset()
.We implement this
destroy
in the localStorage platform, but it shouldbe implemented accordingly to deal with other storage context, such as
with indexedDB for workers or async-storage for react-native