Skip to content

Use async storage instead of localstorage#9919

Merged
darkwing merged 1 commit intoMetaMask:developfrom
darkwing:async-storage
Nov 24, 2020
Merged

Use async storage instead of localstorage#9919
darkwing merged 1 commit intoMetaMask:developfrom
darkwing:async-storage

Conversation

@darkwing
Copy link
Copy Markdown
Contributor

@darkwing darkwing commented Nov 19, 2020

This may help with #9821

Explanation:

At present, we're setting and getting lots of data via localStorage, which is synchronous and can be slow. This is an attempt at using indexedDB and make everything async.

@darkwing darkwing requested a review from a team as a code owner November 19, 2020 22:25
@darkwing darkwing requested a review from Gudahtt November 19, 2020 22:25
@darkwing darkwing marked this pull request as draft November 19, 2020 22:25
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 19, 2020

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

darkwing
darkwing commented Nov 19, 2020
View reviewed changes
@darkwing darkwing marked this pull request as ready for review November 20, 2020 16:35
@darkwing darkwing changed the title WIP: Use async storage instead of localstorage Use async storage instead of localstorage Nov 20, 2020
Gudahtt
Gudahtt reviewed Nov 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

This looks fantastic! The new storage-helper API is much nicer.

Even if this doesn't help the related issue at all, using IndexedDB over Local Storage seems like an improvement, so that we aren't synchronously saving large objects.

@brad-decker
Copy link
Copy Markdown
Contributor

brad-decker commented Nov 21, 2020 •
edited
Loading

Q: I'm wondering if we should move the new module into /shared/modules and replace the usages of local storage in the background process as well? Would this work in an extension's background process? Not in this PR, though, this should land and then maybe consider elevating it to be shared.

@Gudahtt

This comment has been minimized.

Sign in to view
@darkwing darkwing dismissed a stale review via cfd1848 November 23, 2020 18:37
@darkwing
Copy link
Copy Markdown
Contributor Author

darkwing commented Nov 23, 2020

Q: I'm wondering if we should move the new module into /shared/modules and replace the usages of local storage in the background process as well? Would this work in an extension's background process? Not in this PR, though, this should land and then maybe consider elevating it to be shared.

I'll experiment with that. It should work seamlessly, as local-forage falls back to localStorage when IndexedDB isn't available.

Gudahtt
Gudahtt approved these changes Nov 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

We don't use the local storage API in the background, so this seems fine in just the UI for now. The background uses the similar-but-different storage.local extension API, which is already async.

@darkwing darkwing merged commit bf65c97 into MetaMask:develop Nov 24, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

Assignees

@Gudahtt Gudahtt

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@darkwing @brad-decker @Gudahtt