Skip to content
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

Switch to IndexedDB #26

Open
telesvar opened this issue Jun 9, 2020 · 4 comments
Open

Switch to IndexedDB #26

telesvar opened this issue Jun 9, 2020 · 4 comments

Comments

@telesvar
Copy link
Contributor

telesvar commented Jun 9, 2020

Hello, thank you for this project! I want to leave an issue to let you know that I am working on switching from local storage to IndexedDB. Here are the reasons for doing so:

  1. By leveraging IndexedDB, one can use firebase-auth-lite in Service Worker setting;
  2. Browser extensions work better with IndexedDB.

The benefits above are also communicated by the Firebase team.

Also, what do you think of making IndexedDB default and only storage?

@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label feature_request to this issue, with a confidence of 0.56. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@samuelgozi
Copy link
Owner

@aidarkhanov Thank you for submitting this pull request.
Initially, I decided to go with local storage because of the better browser support(specifically opera mini which is widely used in Africa).

However, you made some good points. Since I do intend on using service workers in the near future with this SDK it would make sense to move to IndexedDB, the only issue I see there is that the API is very difficult to work with, and using a library like IDB will double the size of this lib.

It is currently very easy to extend this library to work with other storage solutions. The constructor of Auth receives a storage property that should be an object with the signature:

interface Storage {
	/** Save to local storage */
	set(key: string, value: string): Promise<void>;
	/** Get from local storage */
	get(key: string): Promise<string | null>;
	/** Remove from local storage */
	remove(key: string): Promise<void>;
}

And the current implementation is close to:

const storageApi: Storage = {
	async set(k: string, v: string) {
		return localStorage.setItem(k, v);
	},

	async get(k: string) {
		return localStorage.getItem(k);
	},

	async remove(k: string) {
		return localStorage.removeItem(k);
	}
};

So you can use this as a reference for IndexedDB.

To recap; I will switch to IndexedDB in the future(probably before v1.0.0) but I can't yet tell you exactly when since I'm currently focused on refactoring firebase-firestore-lite, only after I will switch my focus to this lib.

Please let me know if you have any suggestions. I will leave this issue open for discussion and untill it is implemented(or otherwise).

@andymatuschak
Copy link

To recap; I will switch to IndexedDB in the future

FWIW, there are some contexts where local storage will remain the better option: for example, Safari forbids IDB access in iframes. I like your dependency injection approach!

@samuelgozi
Copy link
Owner

@andymatuschak thank, I didn’t know that. I will take that into consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants