Add unit tests for BaseStore

[jakolehm]

• adds unit tests for BaseStore
• removes misleading async from onModelChange & saveToFile

[ixrock]

Might be the issue related to sindresorhus/conf#114 ?

Currently BaseStore.saveToFile() looks like this:

  protected async saveToFile(model: T) {
    logger.info(`[STORE]: SAVING ${this.path}`);

    // todo: update when fixed https://github.com/sindresorhus/conf/issues/114
    if (this.storeConfig) {
      for (const [key, value] of Object.entries(model)) {
        this.storeConfig.set(key, value);
      }
    }
  }

I don't know how internally conf work, but here we have to create multiple savings to file instead of one model update.

[jansav]

I feel like this isn't the solution here. For me, it looks that the real issue is that onModelChange is called multiple times. Not sure though.

[ixrock]

If writing to the storage file is sync why it could be a an issue?

[jakolehm]

If writing to the storage file is sync why it could be a an issue?

onModelChange can be triggered multiple times, if we don't await then the order of execution of sync writes might happen out of order.

[ixrock]

Ok. What if remove async from onModelChange? And probably from saveToFile.

[jakolehm]

Ok. What if remove async from onModelChange? And probably from saveToFile.

It works as well but it modifies api behaviour (this class is exposed via extension api).

[ixrock]

@Nokel81 yes, probably you're right.
Initially I thought there could be some timing issues, but maybe not..
This code produces expected result:

class Foo {
  constructor(){
    this.bar();
  }

  bar(): void {
    console.log("in Foo.bar");
  }
}

class Bat extends Foo {
  async bar(): Promise<void> {
    await new Promise(resolve => setTimeout(resolve, 500));
    super.bar();
    console.log("in Bat.bar");
  }
}

new Bat();

OUTPUT:

[LOG]: "in Foo.bar" 
[LOG]: "in Bat.bar" 

[jakolehm]

Outcome of this discussion: we need to remove async from these functions so that reaction will be sync.

@ixrock @Nokel81 right?

[Nokel81]

That is what I believe yes.

[ixrock]

@jakolehm yes, it could be best solution I guess.

[jakolehm]

I have been testing different things and I'm quite certain now that this PR mainly just adds tests for BaseStore, nothing actually changed in the execution order. I still removed async because it's quite misleading in this context.