More control over models synching

[nathanlepori]

Right now models are synched in the constructor when using the sync option and the promise returned when calling model.sync is ignored. It would be better to have a guard.sync method that accepts the same options as the underlying sequelize method.

[pankajvaghela]

Thanks @nathanlepori , have you try to find a workaround? or what can work..

I am bit out of touch with this project so i will have to dig a bit to find solution

[nathanlepori]

RN I'm trying to see how it can be done here. The problem is that in the original code getCache() gets called in the constructor, and ideally all async functions should be called somewhere else since constructors can't return promises. If the new sync() is called afterwards this isn't a problem since getCache is called there after syncing. Otherwise the cache is not going to match what's in the database.
I have three possible solutions for that but they all are going to change the codebase a bit.

1. Adding a async init() method that does the cache initialization and has to be called before the guard can be used. Of course this has to be documented.
   1.a. The method may even be static and the constructor throw an error if called directly, so that the only way to create an instance is through the init() method.
2. Using lazy initialization: when something from the cache is requested, a flag is checked to see if the cache needs to be initialized, and if needed the result is returned as a promise after the initialization. For this to work all cache access needs to be made async, but most of the time (after the first initialization) it would be accessed synchronously anyways. AFAIK cache access is only done inside the guard's event emitter callbacks, so that wouldn't really change how the library is used...
3. There is a third solution to solve the problem of having an async constructor that I just found myself (dunno if someone found a similar solution anywhere else). It is a bit "hacky" and unconventional but it serves the purpose pretty well IMO. Below is a snippet of how it's implemented:

function createAsyncConstructor(Class) {
    const AsyncClass = class extends Promise {
        constructor() {
            super(resolve => resolve(Class.constructor()));
        }
    }

    // This solves the problem described here: https://stackoverflow.com/questions/48158730/extend-javascript-promise-and-resolve-or-reject-it-inside-constructor/48159603
    AsyncClass.prototype.constructor = Promise;

    return AsyncClass;
}

class ClassWithAsyncConstructor {
    constructor() {
        this.test = 'Created';
    }

    static constructor() {
        // Do anything async here
        return new Promise(resolve => {
            setTimeout(() => resolve(new ClassWithAsyncConstructor()), 1000);
        });
    }
}

const AsyncClass = createAsyncConstructor(ClassWithAsyncConstructor);

async function main() {
    // Create a new instance from the result of the Promise
    const a = await new AsyncClass();
    console.log(a);
    // Prints ClassWithAsyncConstructor { test: 'Created' } after 1 second
}

void main();

This allows a constructor to return a Promise transparently for the user (as in using the constructor's result as a regular promise that returns a class instance) and of course it works with both async await and .then().

@pankajvaghela Let me know what do you think and I can implement one of the three options no problem.

Edit:
Actually I realized that for option 3 you can return a Promise from a non-ES6 class constructor, hence it's only necessary to inherit from Promise for the typing (SequelizeGuard class is declared as ES6 class in the declaration file), but that doesn't affect functionality.
So the last line in the constructor becomes:

return guard.getCache()
      .then(() => this);

[pankajvaghela]

@nathanlepori I changed the code to following . and it doesn't break anything... but i am not sure what it will help with?

if (opts.sync === true) {
  Promise.all(
    _.map(this._models, (model) => {
      return model.sync({
        logging: guard._options.debug ? console.log : false,
      });
    })
  ).then((syncs) => {
    return guard.getCache().then(() => this);
  });
} else {
  return guard.getCache().then(() => this);
  // guard.getCache();
}

[nathanlepori]

I tried something similar myself and turns out it doesn't really help... Ideally a constructor should only return an instance of the class it is creating, not a promise of said class. Also any promise should be returned to the caller so that any dependent code can be run after the promise resolves.
In this case I think the ideal solution is to move sync and cache initialization outside the constructor. I implemented both changes on this branch (also added some other minor QOL changes like Sequelize 6.1.x compatibility) which I'm using for my own project.
To me it made sense to create a separate sync() method like Sequelize itself does and move the logic there, and that works out of the box.
For cache initialization take a look at this commit. My solution to the problem was moving the calls to getCache() inside the event handlers so that as soon as something tries to use the cache, it is always initialized. I'm quite sure all cases are covered this way, but there might be some edge case I missed. Still, it seems to work so far.
With these two changes no async operation is lost track of, and code that depends on them can run correctly. Only downside is that it is a breaking change for anyone using the library.

Let me know if it makes sense to you. Sorry if i mixed up other features on my branch but I was more concerned with implementing what I needed for myself. Depending on whether you intend to still depend on sequelize@5 I can rollback those changes and create a PR.