Document methods are not bind to the document

[rafamel]

Document methods are not hard bind to the document on 8.0.0-beta.7, so if:

db.collection({
  // ...
  methods: {
    hello() {
      console.log(this);
    }
  }
});

Then:

doc.hello(); // RxDocument...
const hello = doc.hello;
hello(); // undefined

This is not the case for statics, nor was it for v7.

[pubkey]

I could not reproduce with the attached test.
Here the binding seems correct.

[rafamel]

For the following code I'm getting an output:

In: hi
Out: 

const RxDB = require('rxdb');
RxDB.plugin(require('pouchdb-adapter-idb'));
RxDB.plugin(require('pouchdb-adapter-memory'));

async function test() {
  const db = await RxDB.create({
    name: 'testdb',
    adapter: 'idb',
    multiInstance: false
  });

  const schema = {
    version: 0,
    type: 'object',
    primaryPath: '_id',
    properties: {
      name: {
        type: 'string'
      }
    }
  };

  const collection = await db.collection({
    name: 'person',
    schema: schema,
    methods: {
      hello() {
        return this.name;
      }
    }
  });

  // Insertion
  await collection.insert({ name: 'hi' });

  const item = await collection.findOne().exec();
  const hello = item.hello;

  console.log('In:', item.hello());
  console.log('Out:', hello());
}
test();

[pubkey]

In your test you take the method from the prototype and run it without and instance.
This never works, for example:

function MyClass() {
    this.x = 'foobar';
}
MyClass.prototype = {
    hello() {
        return this.x;
    }
};

const instance = new MyClass();

console.log('one:');
console.log(instance.hello());

const helloMethod = instance.hello;

console.log('two:');
console.log(helloMethod());

Will give

one:
foobar
two:
init.test.js: unhandledRejection
TypeError: Cannot read property 'x' of undefined

You could fix this by binding the instance afterwards

const helloMethod = instance.hello.bind(instance);
console.log('two:');
console.log(helloMethod());

[rafamel]

Yes, you are correct, that's what we'd expect for any class.

However, this was not the case (somehow) on v7. It's also not the case for RxCollection's statics (so I assume there's some explicit binding going on). Also, when having methods on an item (RxDocument), I can see the use case for passing around the method without the document when using it in components (ex. I have a setName method and want to pass it to an input component) - this is something I'd regard as common.

I'm assuming the change is now methods are inherited from the prototype when previously they were directly assigned to the document object? I can see the performance gain in that, if it's the case, so losing this and having to explicitly bind methods would be reasonable in exchange.

Though something that could be done is to set a getter for each method in the prototype such as:

  get methodName() {
    return originalMethodFn.bind(this);
  }

This would allow us to have the same performance gain when creating documents as the getter would be part of the prototype, and would only fire when accessed. At the same time, accessing that property would return the original method bind.

Following your example:

function hello() {
  return this.x;
}
function MyClass() {
  this.x = 'foobar';
}
MyClass.prototype = {
  get hello() {
    return hello.bind(this);
  }
};

const instance = new MyClass();
const helloMethod = instance.hello;

console.log('one:', instance.hello());
console.log('two:', helloMethod());

one: foobar
two: foobar

It would need to be done programmatically, so something like:

const methodGetters = Object.entries(methodsObj).reduce(
  (acc, [key, method]) => {
    acc[key] = {
      get() {
        return method.bind(this);
      },
      enumerable: true
    };
    return acc;
  },
  {}
);
Object.defineProperties(RxDocumentPrototype, methodGetters);

Applied to your example:

const methodsObj = {
  hello() {
    return this.x;
  }
};

function MyClass() {
  this.x = 'foobar';
}

const methodGetters = Object.entries(methodsObj).reduce(
  (acc, [key, method]) => {
    acc[key] = {
      get() {
        return method.bind(this);
      },
      enumerable: true
    };
    return acc;
  },
  {}
);
Object.defineProperties(MyClass.prototype, methodGetters);

const instance = new MyClass();
const helloMethod = instance.hello;

console.log('one:', instance.hello());
console.log('two:', helloMethod());

I'd personally advocate for it being the default, as I can't see any reasonable downside, but otherwise it'd be very useful to have it as an option regardless.

[pubkey]

Yes this does not work in v8 because we set the orm-methods at the prototype, not for each RxDcoument. The direct binding in v7 was an affect, not an intended use.

You proposal to bind when the getter is called would work, but only for the ORM-methods.
For example you could not do the same with RxDocument.remove() or other methods of RxDocument.
I think this would be confusing and officially supporting this will bribe people into passing arround functions without using .bind().

Your described use case is legit but this is what the postCreate-hook is for.

You could get the same with

function setName(){
  return this.doSomethingWithThis();
}

myCollection.postCreate(function(plainData, rxDocument){
  rxDocument.setName = setName.bind(rxDocument);
});

[rafamel]

Hi @pubkey , really appreciate the answer. Let's look over your points.

Regarding explicit binding via hooks or plugin, doing so was the first thing I tried before opening the issue. Those properties are protected by RxDB and marked as read only, so it'd only be possible to do by monkey patching the functions that do RxDocument creation via plugin. Since those are private, I don't think doing that is a great idea since it could break any time the internals change.

You also point out that the "proposal to bind when the getter is called would work, but only for the ORM-methods". I'm not fully understanding your point here - as far as I can see it, it could be applied to any method, both user defined and ORM ones (remove, create, and so on). If we have the user defined methods property for the collection, and we have a ORMmethods object with the ORM methods, it would work like so:

const ORMmethods = {
  remove() {
    // ...
  }
  // ....
};
const methods = {
  hello() {
    return this.x;
  }
  // ....
};

const createGetters = (obj) => Object.entries(obj).reduce(
  (acc, [key, method]) => {
    acc[key] = {
      get() {
        return method.bind(this);
      },
      enumerable: true
    };
    return acc;
  },
  {}
);
Object.defineProperties(Document.prototype, createGetters(ORMmethods));
Object.defineProperties(Document.prototype, createGetters(methods));

Last, you mention that it "would be confusing and officially supporting this will bribe people into passing arround functions without using .bind()." This is essentially the whole point, that they (we) don't have to think about binding. The confusion in reduced, rather than increased, in my view. This is the reason why on React it is common practice to set component methods as arrow functions instead of directly binding them (so we don't have to think about binding, and we can pass them around). Unfortunately, due to the way methods are defined in RxDB, that's not a possibility. It's also the reason why widely used state libraries like mst bind it via closure.

Taking mst as an example, when defining methods (actions), they pass a self object, so methods are bind via closure. This is an approach that would be, in my view, also reasonable as an alternative to the getters method. The api would be something like:

  const collection = await db.collection({
    // ...
    methods: (selfOrThis) => ({
      hello() {
        return selfOrThis.name;
      }
    })
  });

Unfortunately, contrary to the getters way, this wouldn't allow for ORM methods to be bind, unless explicitly creating a method for it (for example, creating a del method that calls remove and so on). So if going this way ORM methods should probably be bind by default (this is, for example, what React does; it binds lifecycle methods to this -component-, while custom methods are bind by properties+arrow function).

I'm personally passing around a lot of methods, and would imagine others are too. Making it so there's a need to think about binding on each single instance you use any method from RxDB (as opposed to any other state library) I'd consider a strong hassle and a very pronounced downside to the library as a whole. I'd urge to consider that there's a chance that by not wanting to introduce complexity to the library, we might actually be doing so to a higher degree.

To recap, whether it is via getters or via closure + ORM methods bind by default, I'd put forwards there should be a way by which RxDB allows us to not have to explicitly bind methods on usage. I'm not sure how other libraries pass around methods, but in the case of React, I can assure you this will be a very relevant point for many people using RxDB with it.

[pubkey]

I am convinced.
As you layed out, my first argument is invalid, the binding can also be done for the other methods of RxDocument.
I never understood why react-people define prototype-methods as arrow-functions (handleClick = () => {..), I thought it was a bad practice. No I understand 😃

In your example-code you set enumerable: true, is there a specific reason for this?

[rafamel]

That's great news!! Really glad about this -really, you have no idea.

All right, regarding enumerable, every object property descriptor has possible properties value, set, get, enumerable, writable, and configurable. These are the ones that can be set by Object.defineProperty()/Object.defineProperties().

Getters, when set on object definition, get both enumerable: true and configurable: true. When setting it via defineProperties() these will be set as false by default and must be set as true, if desired, explicitly.

As an example, given:

const obj1 = {
  get some() {}
};

const obj2 = {};
Object.defineProperties(obj2, {
  some: {
    get() {}
  }
});

If we did Object.keys() on obj1 we'd get ["some"], while if we did it over obj2 we'd get an empty array, as the only property we defined wasn't explicitly set as enumerable.

If we were to do Object.getOwnPropertyDescriptors() over both we'd get:

// obj1
{ some:
   { get: [Function: get some],
     set: undefined,
     enumerable: true,
     configurable: true } }
// obj2
{ some:
   { get: [Function: get],
     set: undefined,
     enumerable: false,
     configurable: false } }

However, with enumerable: true:

const obj = {};
Object.defineProperties(obj, {
  some: {
    get() {},
    enumerable: true
  }
});

console.log(Object.keys(obj));
console.log(Object.getOwnPropertyDescriptors(obj));

// Object.keys()
[ 'some' ]
// Object.getOwnPropertyDescriptors()
{ some:
   { get: [Function: get],
     set: undefined,
     enumerable: true,
     configurable: false } }

All in all, I was just setting enumerable: true so they are enumerated when iterating over the object keys -I assumed this is something we'd want, though it wouldn't think of it as critical. We're not setting configurable: true as I assumed we don't want the property descriptor on the prototype to be changed once set.

[pubkey]

Ok. I initaly set enumerable: false because it would anyways only appear when you run Object.keys on the prototype.

So I think I can close this. Next beta-release will come when travis has run throught.

[rafamel]

Sounds great!

[rafamel]

Hi @pubkey , I think there's a case for enumerable: true we hadn't thought about. When console.logging the instance of a class, if enumerable: false, properties won't show as own (example - ignore the ones I se manually: data, store, units, views, and both Symbol). Even though it is a minor issue, thinking about this further, it would be ideal if we set enumerable: true for all property getters and methods, and enumerable: false for all subscribers and privates (_property, _property_) so they don't pollute as much.

What do you think?

[pubkey]

Great observation. Changed, now looks like this

[rafamel]

Great!