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

Local Forage support #278

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

Mogball
Copy link

@Mogball Mogball commented Dec 18, 2017

#155
#153

This is my attempt to add localforage into ember-local-storage.

A few things to note:

  • localforage is an async-only API, which means that the API for ember-local-storage has to be async (otherwise one risks dealing with race conditions)
  • localforage lacks a storage event as mentioned in Discussion: Local Forage, moving beyond LS #153; there is a localForage-observable package that functions across tabs and essentially does the same as the storage event, but this package is not available on bower because one of its dependencies is not

Those things considered, it might do better as a fork. On the other hand, commit 548b361 seems to function okay without the async API (testing in an actual ember app) but a few test cases weren't dealing with race conditions well.

@fsmanuel
Copy link
Member

@Mogball wow. Thanks. I'll review as soon as possible!

@Mogball Mogball force-pushed the jeff/localforage branch 2 times, most recently from 9a24e47 to c6a66a8 Compare December 19, 2017 23:19
@akshaisarma
Copy link

@fsmanuel, I would love to have this change merged in. @Mogball actually contributed this code for my project's use case mentioned here. Right now we're using @Mogball's fork directly but would prefer to use the official ember-local-storage plugin. If there's anything I can do to help in this review, please let me know.

@fsmanuel
Copy link
Member

fsmanuel commented Feb 7, 2018

@Mogball @akshaisarma I didn't had time to review yet. The main problem is that it changes the api to async. That would break every app that uses the addon. At least that would require a manjor release. My goal is to have localForage as an option that can be configured.
Something like:

// config/environment.js
module.exports = function() {
  var ENV = {
    'ember-local-storage': {
      useLocalForage: true
    }
  }
};

That way we can add a section to the readme that explains how the async api works if localForage is enabled.

To make that possible we need some wrapper fuctions that abstracts the sync ans async behavior. At a first glance @Mogball implementation looks good and we can use all that code for the async/localForage use.

I'll ping you as soon as I have done the review.

Removed bower dependencies
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.

3 participants