stampit-org / stampit

OOP is better with stamps: Composable object factories.
https://stampit.js.org
MIT License
3.02k stars 102 forks source link

ES6 Arrow Functions #264

Closed koalabz closed 7 years ago

koalabz commented 7 years ago

First of all, thanks for this amazing piece of code. I'm starting playing with stampit and i'm stuck trying to use arrow functions in methods() function like this :

const stamp = stampit().methods({
  error: console.error,
  incorrect: (value) => {
    this.error(`value ${value} is incorrect`);
  }
});
stamp().error('bad value') // ok 
stamp().incorrect('bad value') // TypeError: Cannot read property 'error' of undefined

I understand that this do not point to the stamp object itself. Is there anyway to get the same arguments : { instance, stamp, args } like the init() function ?

koresar commented 7 years ago

No worries :)

This is not a valid code. Here I fixed it:

const stamp = stampit().methods({
  error: console.error,
  incorrect(value) { // < -THAT LINE
    this.error(`value ${value} is incorrect`);
  }
});

Now the this is pointing to the object instance. Basically, that's how JavaScript works.

Cheers!

koalabz commented 7 years ago

Thanks @koresar for your quick answer but why not use the same approach than init() for methods() func like this :

const stamp = stampit().methods(({ instance, stamp, args }) => ({
   error: console.error,
   incorrect: (value) => {
      instance.error(`value ${value} is incorrect`);
   }
));

methods() accept also a function with the same init() options object and return methods, but we can use ES6 Arrow Func

danielkcz commented 7 years ago

@koalabz You really like that better than what @koresar showed you? It's terribly verbose :) Personally I don't use fat arrows with stampit at all to avoid confusion and errors.

Either way it's not really possible because in the time when you are calling methods, there is yet no instance to refer to. These methods are attached to descriptor and used for any future instance of that stamp. You could only solve this by using configuration and creating these methods within initializer dynamically, but that's hardly a same thing.

If you are familiar with AST and writing Babel plugins, you can perhaps make the one that can handle this properly

koalabz commented 7 years ago

@koalabz You really like that better than what @koresar showed you?

I don't prefer one or the other i just try to understand why we can't use ES6 Arrow in other methods than init function.

Either way it's not really possible because in the time when you are calling methods, there is yet no instance to refer to.

It's the same when you call initmethod right ? As reference in doc API: create(), object is created when you invoke createor execute your stamp.

Without doubt the arrow function is a great addition. When used correctly it brings simplicity in places where earlier you had to use .bind() or trying to catch the context. It also makes the code lighter. Advantages in some situations brings disadvantages in others.

:)

koresar commented 7 years ago

So, yeah. Daniel is correct. If you want to use stamp and args in the other methods you'd need to attach them to the object instance. And then use them as this.stamp or this.args in your methods.

Also, don't put arrow functions everywhere. ☺️ It's not a silver bullet.

koresar commented 7 years ago

We can't use arrow functions as methods because arrow functions bind the "this" context to the upper scope. In your case it's the "undefined". You need to remove that arrow function as I suggested and you'll be get access to the "instance" as the "this" context inside your methods.

koalabz commented 7 years ago

Thanks @koresar @FredyC for your answers i will use the old style this as mentioned above 😄 Try to understand deeper stamp-specification when have more time .

danielkcz commented 7 years ago

@koalabz Check this out :)

const stamp = stampit().methods({
  oldStyle: function() { ... },
  fatStyle: () => { ... },
  newStyle() { ... },
});

So regarding your quote about arrow function bringing simplicity, I am sure you can see that in this case it only brings chaos :)

Closing as it's not possible to solve this on library level.

neverfox commented 7 years ago

I just wanted to second @koalabz's suggestion because I would prefer never to use this and I'm willing to pay a price in verbosity to avoid it. I'm curious why, @FredyC, it's not possible on a library level to allow that form. Wouldn't it just be a matter of seeing if the argument to methods is a function and, if it is, use the returned value?

koresar commented 7 years ago

@neverfox could you please show us code example of the proposed syntax? Otherwise, I struggle to understand code not seeing code. :)

ericelliott commented 7 years ago

Arrow functions do not bind this -- in fact, they can't bind this because they don't have their own slot for binding this at all. Arrow functions always delegate this to the lexical scope.

In other words, if you want to declare a method that uses the instance, you should chose one of these options:

  1. Define it in an init function (which gets the instance passed in).
  2. Define it with a standard function or method syntax (which can access the instance as this).

If you want to avoid this, you should also completely shun instance state, and instead use closures for state -- which means you should be using initializers to define privileged methods.

ericelliott commented 7 years ago

@neverfox You can already avoid this -- simply define all your methods as privileged methods inside init functions, and you'll get the instance passed in. You can't do that with methods because by definition in JavaScript, methods use dynamic this binding. There's no way the library could assure that the instance is available without forcing some convention, double-delegating all method calls, and injecting an instance parameter.

That would be a breaking change, it would dramatically alter the behavior of all methods, cause a performance hit for every method call, and cause a TON of confusion. In other words, it's not gonna happen.