keystonejs / storage

Storage.js is a Javascript library that gives you an easy and standardised access to any provider of your choice.
MIT License
60 stars 12 forks source link

Easier hook init #9

Closed grabbou closed 10 years ago

grabbou commented 10 years ago

It'd be better to declare hooks like so:

// provider-specific
Storage.pre('provider', 'upload', function (next) {
    next(err);
});

// and in the background
Storage.pre = function (provider, method, hook) {
    if(!this._cache[provider]) // call the constructor of provider without calling _init
    this._cache[provider].pre(method, hook);
});

// global - declared for all of the providers configured
Storage.pre('upload', function (next) {
    next(err);
});

// in the background inside Storage class
Storage.pre = function (method, hook) {
   if(!this._cache.length) // call the constructor of default
   _.forEach(this._cache, function (client) {
      client.pre(method, hook);
   });
});

instead of getting storage client in async manner like we do now:

Storage.get('provider', function (err, client) {
   client.pre('upload', function(next) {
        next(err);
   });
});

What you think? Similar code for post hook.

In first case - we simply check whether provider we want to hook is already defined. If not -> we create it by calling its constructor (please note that we do not call _init method so that operation is synchronous). If it's not available - error is being thrown from Storage.get('provider') that's unable to obtain an instance.

In second case - we declare hooks for all of the providers available in our cache. If nothing is available -> we try to declare default provider (so we perform Storage.get() in the background without _init method). If default provider is not available - same as above.

Async _init method will be invoked on first Storage.get() called by the user.

Method and arguments are subject to discuss and probably -> change.

webteckie commented 10 years ago

@grabbou personally I like the simplicity of the proposed syntax. You may even consider something like this(?):

Storage.provider.XXX.pre('upload', function (next) { next(err); });

grabbou commented 10 years ago

@webteckie, thanks for your input! Unfortunately, we can't write here dot access as under Storage.providers we have constructors of available providers to use. Writing this kind of access may occur multiple errors for users as they might try to write something like this Storage.provider.XXX.upload(...). In some cases, they might forget about _init() method meaning for example that they will try to upload a file to MongoDB database without even opening connection to it.

Leaving open for now as one extra thing is missing.

To clarify:

Problem: _If generic version called, we only check whether _cache size is bigger than 0. If yes -> we simply iterate through it and hook methods of every instance inside it._ If not -> we simply try to get default instance using _getInstance that will automatically raise an error if something is wrong. That method will update our _cache automatically.

Bug: User configured 5 providers and already initialised one of them (it means that our cache is not empty). Current version will add hook to that one only skipping the rest 4 providers configured but not yet created/initialised.

Solution: We have to compare _cache against _config to make sure that every provider configured has been already created (doesn't matter whether it's been initialised or not).