Add support for other storage options

[joaogranado]

There are some issues and pull requests that try to fix this by implementing other storage managers, based on Web Storage API. I think we should think about a different approach, which would be simpler to maintain, and would lead to a more user-friendly API. It would be a major breaking change, but would simplify the API by removing the dependency on $cookies service. My suggestion takes some clues from reflective meta programming, and it is quite simple. I consists on redefining the semantics of the storage operations by passing a target object, such as $cookies it would be possible to map its methods to an abstract storage service. Example:

Configuration remains the same.
// oauth-token-config.js
myModule.config(OAuthTokenProvider => {
  OAuthTokenProvider.configure({
    name: 'token',
    options: { secure: true }
  });
});

Using $cookies. This should be done in a run block to allow injecting third party dependencies.

// oauth-token-run.js
myModule.run(($cookies, OAuthToken) => {
  // Cookies example.
  OAuthToken.extend($cookies, config => {
    return {
      getToken: {
        method: 'getObject',
        args: [config.name]
      },
      removeToken: {
        method: 'removeToken',
        args: [config.name, config.options]
      },
      setToken: data => ({
        method: 'putObject',
        args: [config.name, data, config.options]
      })
    }
  }));

Using sessionStorage:

// oauth-token-run.js
myModule.run(() => {
  // sessionStorage example.
  OAuthToken.extend(sessionStorage, config => {
    return {
      getToken: {
        method: 'getItem',
        args: [config.name]
      },
      removeToken: {
        method: 'removeItem',
        args: [config.name]
      },
      setToken: data => ({
        method: 'setItem',
        args: [config.name, JSON.stringify(data)]
      }),
    }
  }));

The implementation of the OAuthToken would be straight forward, such as:

class OAuthToken {
  extend(target, handlers) {
    this.storageService = target;
    this.handlers = handlers(config);
  }

  setToken(data) {
    // We should execute validations to check it the storage service was set.
    const { args, method } = this.handlers.setToken(data);

    this.storageService[method](...args);
  }

  getToken() {
    const { args, method } = this.handlers.getToken;

    this.storageService[method](...args);
  }

  removeToken() {
    const { args, method } = this.handlers.removeToken;

    this.storageService[method](...args);
  }
}

As I've mentioned in the comment on setToken we should validate on each method if both target and the required method were defined. This might seem a lot more configuration, and should be well documented, but this solves the following recurrent issues:

• Removes dependencies from third-party storage libraries - $cookies.
• Avoids maintaining a large code base prone to bugs, and not scalable, with naive implementations, that don't take basic security considerations.
• Adds flexibility to implementing other modules than Web Storage (both localStorage and sessionStorage) and cookies.

@ruipenso what do you thing? It would be awesome if someone this :)

[joaogranado]

Another option would be adding $cookies as a default option for storage management and instead of the more verbose approach explained above, we could simply extend the default method such as:

// oauth-token-run.js
myModule.run(() => {
  // sessionStorage example.
  OAuthToken.addStorage({
      getToken: config => sessionStorage.getItem(config.name),
      removeToken: config => {
        sessionStorage.removeItem(config.name)
      },
      setToken: (config, data) => {
        sessionStorate.setItem(config.name, JSON.stringify(data))
      }
  }));
});

class OAuthToken {
  addStorage(storage) {
    this.storage = storage;
  }

  setToken(data) {
    this.storage ? this.storage.setToken(config, data) : $cookies.putObject(config.name, data, config.options);
  }

  getToken() {
    return this.storage ? this.storage.getToken(config) : $cookies.getObject(config.name);
  }

  removeToken() {
    this.storage ? this.storage.removeToken(config) : $cookies.remove(config.name, config.options);
  }
}

Out of the box it would simply work with $cookies, which IMHO should be the default storage service, and solves some hassles related with handling arbitrary data using cookies. I would provides a more simple API to extend storage service. It is way more explicit than creating an abstraction based on a strategy architecture, which would leave to us the responsibility of fixing future issues with things that are out of the scope of this library.

[jonkoops]

For those following this thread please consult the discussion in #95

[jonkoops]

I think the currently described implementation still undesirable because it exposes the implementation to the user too soon. Alternative storage solutions should be included in the library itself and they should be first class citizens.

It is essential that storage methods are defined in a way that allows them to be used as a fallback if the method storage before them is not supported. For example: Cordova/PhoneGap has no support for cookies so it could fall back to web storage, or Internet Explorer 9 and lower has no support for web storage should fall back to cookies.

It is important that the user remains in control of the order of storage solutions see #95 (comment), but we should provide the defaults this library has always had. So cookies first, minimising the impact on existing users.

Providing a structured way of registering storage methods makes it easier to create a public API in the future and lets the community create custom storage solutions. Cookies would be just another implementation of this storage interface which looks a lot like the code @joaogranado posted above, without the explicitness of what storage solution is used.

A storage method is either supported or not, and the first supported method provided by the user or the defaults is used. This makes the final storage definition more composable.

[joaogranado]

What you're proposing is to implement angular-local-storage internally, which unfortunately we couldn't accept as a reasonable solution.

[jonkoops]

So to demonstrate in code:

// Configuration with defaults.
myModule.config(function (OAuthTokenProvider) {
  OAuthTokenProvider.configure({
    name: 'token',
    storage: ['cookie', 'session', 'memory'], // Falls back to next storage method until `isSupported()` is true.
    options: { secure: true }
  });
});

// Example implementation of session storage. Provided with library.
myModule.run(function (OAuthToken) {
  OAuthToken.registerStorage('session', {
    isSupported: function (options) {
      var testKey = '_supported';

      try {
        localStorage.setItem(testKey, testKey);
        localStorage.removeItem(testKey);
        return true;
      } catch(err) {
        return false;
      }
    },

    setToken: function (name, token, options) {
      sessionStorage.setItem(name, JSON.stringify(token));
    },

    getToken: function (name, options) {
      var token = JSON.parse(sessionStorage.getItem(name));

      if (token === null) {
        return;
      }

      return token;
    },

    removeToken: function (name, options) {
      sessionStorage.removeItem(name);
    }
  });
});

[jonkoops]

@joaogranado I think it's more than acceptable. The real complexity is in the cookie storage method, which requires lot's of code to parse and write cookies. For this we could continue to use ngCookies until possible deprecation of cookies as a storage method. Or even better, move it to a seperate module that registers itself as a storage solution. This would also remove the need for the dependencies in the current project and give more power to the user when needed.

The process of registering different storage solutions is way more DRY than having to check for each storage solution in the implementation of OAuthToken. And the code to implement it shouldn't need to be complex.

[joaogranado]

So you are agreeing with me that we should provide a easier way to extend the component, right? What I propose doesn't introduces backward compatibility issues, and it is not a breaking change, but a feature instead. It could be already accomplished with a module.decorator, by extending the module, so we could consider this as adding syntactic sugar. I don't agree with adding the fallback order because it would be also add some complexity, even with a default configuration. You could use an external library as angular-local-storage, where you can already define that order, and it would lead to a more modular implementation. Despite the fact that adding a isSupported method looks good to me, it would be covered by an external dependency.

[jonkoops]

Yes, I completely agree we should provide an easier way to extend the functionality. My proposed solution is backwards compatible as well, let there be no doubt about that. It also has the added benefit of making it easier to implement new storage solutions and optionally putting the user in control of which solutions are used. Which is kind of what the problem is all about. I have no control nor are the defaults sensible for me (and this is true for others that have reported similar bugs, pull requests and forks).

These are the requirements for me:

• Sensible defaults matching the current implementation to ensure backwards compatibility.
• Let the user able to specify which storage methods should be used, if they so please.
• Make it easy to register storage methods so they can be modularised in the future (makes it possible to run without ngCookies).
• Fall back to the next configured storage method automatically until a method is supported or until there are no more methods to test support for.
• Possibly remove the hard dependency on ngCookies by logging a warning when isSupported() is called on the cookie method is used but ngCookies is not included.

The thing I am fighting for here is basically to allow the user to progressively enhance their app the way they want and have it 'just work' for those who don't bother. Cookies aren't always available and neither is web storage, and users shouldn't suffer because of it. Most people will not implement custom storage and they shouldn't need to.

[jonkoops]

I haven't received any feedback so I am assuming there isn't any interest in having the proposed method implemented. I've forked this repository for my own purposes so I can run in Cordova without having to jump trough hoops. I would still very much like to implement the method I've discussed so if there is interest please let me know.

[ruipenso]

@jonkoops @joaogranado Sorry, but only now I had time to read this discussion.

Having a way to extend/register different storage mechanisms would be great, but I don't think that's what our users really need or want.

We must keep it simple by allowing the users to just switch between storage managers (it could be an array to fallback to the next one).

For this, we can implement a storage manager registry underneath to help implement more managers in the future. But, at the end of the day, our users just want to get rid of cookies and use another one.

As @joaogranado pointed out, it's basically what angular-local-storage does and I don't think it's a good idea to have more code to maintain. They are on the verge of merging the cookies' secure option, so we can probably use it or, IMO we should keep using ngCookies and implement localStorage and sessionStorage meanwhile.

At this point, I think it's very important that we don't introduce any BC.

Summing up, I think @jonkoops has a good plan and I would love to see this implemented since it will resolve our number one complaint.

[ruipenso]

Those in favor of the motion, say aye. 😄