fix: fix datastore factory

[hugomrdias]

The factory reference type can't extend the Datastore interface directly.

We want the class instance to implement the interface and not the class (Object) reference.

[achingbrain]

Why would the factory extend the datastore? It should create datastores, not be a datastore.

[hugomrdias]

exactly thats the problem this PR fixes

[achingbrain]

I can see that in the removal of extends Datastore but then why would we set the prototype of the DatastoreFactory interface to be the Datastore interface?

[hugomrdias]

I can see that in the removal of extends Datastore but then why would we set the prototype of the DatastoreFactory interface to be the Datastore interface?

that way TS can infere if a the output of new DatastoreFactory implements Datastore correctly, and the object DatastoreFactory can still be just a abstract object with an constructor.

Without it i can assign a partially implemented DatastoreFactory to a var that will be used to create Datastore and only see error later in the code path or even just when a not implemented method is called.

[achingbrain]

But in the factory pattern a DatastoreFactory would not be a Datastore, it just creates them:

const factory = new DatastoreFactoryImpl()
const datastore = factory.create()

A datastore impl might implement both Datastore and DatastoreFactory, but one interface should not extend the other:

class MyDatastore implements Datastore, DatastoreFactory {
  // ...Datastore methods

  create () {
    return new MyDatastore()
  }
}

[hugomrdias]

humm this isn't actually the factory pattern, its just an extra type for the class object because we can't write class types in .ts files without an implementation. (ts 4.2 will allow for abstract classes in .ts)
So we need a type for the class object with a constructor and a type for the instance interface.

[achingbrain]

If it's not the factory pattern, why is it called DatastoreFactory?

[hugomrdias]

If it's not the factory pattern, why is it called DatastoreFactory?

Suggestions ?

[achingbrain]

Will this not work? ipfs/js-ipfs-repo#275 (comment)

Then you don't need DatastoreFactory at all.

[hugomrdias]

Will this not work? ipfs/js-ipfs-repo#275 (comment)

Then you don't need DatastoreFactory at all.

sure we can do that, and i also confirmed that prototype is not needed.

i would still like to have a type here, i changed the name to DatastoreConstructor, sounds good ?

[achingbrain]

It's a better name, less confusing, but do we really need it or can we delete it entirely?

[hugomrdias]

your suggestion works and is enough, i just think having a type properly defined here can't hurt.
for example in the future if we want to defined params for the constructor we would do it here and dependents would just update instead of having to change it manually

[achingbrain]

for example in the future if we want to..

The YAGNI principle says we should add that when we need it..

[hugomrdias]

gone