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

MLIBZ-2120: Add support for specifying a storage adapter #213

Merged
merged 5 commits into from Jan 26, 2018

Conversation

Projects
None yet
3 participants
@thomasconner
Contributor

thomasconner commented Jan 22, 2018

Changes

Added a storageAdapters option to init() that can be used to select which storage adapter should be used.

Kinvey.init({
  appKey: 'appKey',
  appSecret: 'appSecret',
  storageAdapters: Kinvey.StorageAdapter.WebSQL // This can be an array of storage adapters or a single value
});

Tests

This was tested manually. We should have @tsvetomir-nedyalkov tests this with integration tests.

@thomasconner thomasconner self-assigned this Jan 22, 2018

@thomasconner thomasconner requested review from georgiwe and tejasranade Jan 22, 2018

@georgiwe

@thomasconner , didn't we agree Monday during standup, on some changes to the public API?

I don't personally like the name storageAdapters very much, for a public API. I don't think it should include our name for those classes, or be named after their function. I think it should be what the user perceives it as - the location (or storage mechanism/provider) where their data would be stored.

So to sum up what we discussed on Monday:

  • The top-level setting should have an object value, to facilitate further addition of settings. For the name, I suggest storage, offlineStorage, persistance, localPersistance or something of the sort.

  • The setting for the adapter can be named provider, storageProvider. Something that ties with the above mentioned name.

  • Validation upon initialization would be useful - when you call init() with a value which is not valid for the current platform (not unsupported by the environment, just not an actual option for this SDK), there should be an error saying so. In this implementation, an error is thrown when you try to use the adapter, and the message only states "An error occurred".

  • The namespace for the enum shouldn't pollute the top-level, Kinvey, namespace. Since there isn't a dedicated space for enums and constants, I think it should be on the Kinvey.DataStore object.

@georgiwe

This comment has been minimized.

Contributor

georgiwe commented Jan 23, 2018

I discussed this some with Lyubo and we think it may be worth holding back on the priority list functionality for now. We're not sure it's all that useful and nobody seems to have asked for it yet. And if we release it, we have to support it. How about we just release the single provider selection option for now, and we can think about the list a bit longer?

Here is an example configuration object. Most of it is for the future, but it's the motivation behind having it as an object at an early stage.

const StorageProvider = Kinvey.DataStore.StorageProvider;
const SchemaConstants = Kinvey.DataStore.SchemaConstants;

const storageConfig = {
  provider: StorageProvider.WebSql, // this is all we are currently implementing
  providerConfig: {
    [StorageProvider.WebSql]: {
      maxSize: 1024 * 1024,
      indexes: {
        Books: {
          Title: true,
          ISBN: SchemaConstants.Index.Unique
        }
      }
      // ...
    }
  },
  collectionsSchema: { // we could check out how Mongoose.js does it
    Books: {
      Title: SchemaConstants.Text,
      Author: {
        type: SchemaConstants.Relation,
        targetCollection: 'Authors'
      },
      ISBN: SchemaConstants.Text
    }
  }
};

Kinvey.init({
  appKey: 'some string',
  appSecret: 'another string',
  storage: storageConfig
});
@thomasconner

This comment has been minimized.

Contributor

thomasconner commented Jan 23, 2018

  1. I think Kinvey.StorageAdapters works well but I will change it to something you mentioned.

  2. Same thing as above.

  3. The enum Kinvey.StorageAdapters only contains valid values for the environment you are using. We can validate they don't use a random value that is not in the enum.

  4. I disagree with this. If you put it under Kinvey.DataStore then it implies you are setting the storage adapter only for Kinvey.DataStore. Right now this is the only place we use storage but in the future we might use storage in other areas of the SDK.

@thomasconner

This comment has been minimized.

Contributor

thomasconner commented Jan 23, 2018

What prevents us from supporting a priority list functionality now? Have we discussed the configuration object and I missed it?

@thomasconner

This comment has been minimized.

Contributor

thomasconner commented Jan 23, 2018

Changes

  • I changed Kinvey.StorageAdapter to Kinvey.StorageProvider.
  • I changed the config option for init() to config.storage.
Kinvey.init({
  appKey: 'appKey',
  appSecret: 'appSecret',
  storage: Kinvey.StorageProvider.WebSQL // or [Kinvey.StorageProvider.WebSQL, Kinvey.StorageProvider.IndexedDB]
});
@thomasconner

This comment has been minimized.

Contributor

thomasconner commented Jan 24, 2018

@georgiwe Can you look over the changes I made?

@thomasconner

This comment has been minimized.

Contributor

thomasconner commented Jan 25, 2018

I think all changes requested have been made and this should be ready to go.

@thomasconner

This comment has been minimized.

Contributor

thomasconner commented Jan 25, 2018

@tejasranade Look at the example code snippet and let me know your thoughts. Should we rename the config options storage to something else? Since we refer to this as cache elsewhere should we refer to it as cachePersistence here?

Should the enum StorageProvider be under the Kinvey.DataStore or Kinvey? Should it be renamed to something with cache in the name as described above?

@tejasranade

This comment has been minimized.

Member

tejasranade commented Jan 26, 2018

@thomasconner On the naming convention - storage is good. cachePersistence is misleading (you've noted that it holds more than just the cache) and a little too elaborate. My vote is for storage.

On namespace, again given that storage doesn't just apply to DataStore, I think it's cleaner to put it outside the Kinvey.DataStore context.

I couldn't tell if there were any other open questions outside of these two.

@thomasconner thomasconner merged commit 54d9aa7 into master Jan 26, 2018

@thomasconner thomasconner deleted the MLIBZ-2120_Choose_Database_Adapter branch Jan 26, 2018

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