stampit-org / stamp-specification

The Stamp Specification: Composables
434 stars 16 forks source link

Back to basics with stamp/initializer args #37

Closed markokr closed 8 years ago

markokr commented 8 years ago

Discussion in #35 made me think about argument passing in general. And have a proposal:

Note - all this does not change the fact that single options object is best way to pass args to multiple initializers. All examples showing the code should use that style. But now I see there is no need to hardcode that in spec - why should this spec disallow the React-style 3-arg stamps?

It's based on following ideas:

Here the "no need" means that although for each idea there is pattern that breaks it, there is no such pattern that must be supported by this spec. All the code that uses this spec will be new.

Stampit 1/2 used first arg as properties to be installed to instance, the rest were passed to initializers. Composables drop the idea of direct properties which is good. Now the one and only argument is purely for initializers. This proposal takes it logical conclusion.

koresar commented 8 years ago

Re the instanceof: http://ericleads.com/2013/01/javascript-constructor-functions-vs-factory-functions/#comment-9671 And this: https://github.com/stampit-org/stampit/issues/44

I.e. we explicitly make instanceof to not work. It's an antipattern in OOP.

koresar commented 8 years ago

Re this:

Instance is passed to initializers as this.

It is already passed. Although, we forgot to mention that in the specs: https://github.com/stampit-org/stamp-specification#initializer-parameters :(

koresar commented 8 years ago

Re the instance.constructor - we don't want people to confuse stamps with constructors. This is essentioally different concepts.

koresar commented 8 years ago

Re this:

The stamp factory function will take it's arguments and pass them to initializers directly. No need to extra values.

This is the answer: https://github.com/stampit-org/stamp-specification/issues/26#issuecomment-141719603

koresar commented 8 years ago

I'd agree to that:

  • There is no need to pass instance some other way than this.
  • There is no need for magic keys in options or magic extra args that are specific to stamp launcher. All stamp args belong to initializers.

And even I would agree to that as well:

  • The stamp factory function will take it's arguments and pass them to initializers directly. No need to extra values.

Although, I do not like using the instance.constructor to pass stamp in to an initializer.

markokr commented 8 years ago

Ignore instanceof, it does not matter. But I don't get the following comment: "We don't want people to confuse stamps with constructors. This is essentioally different concepts."

Where is the confusion? Stamp is constructor. And that property is standard way to keep it around.

markokr commented 8 years ago

ANd I know instance is passed as this, my point was that it should be only way to pass it.

koresar commented 8 years ago

Where is the confusion? Stamp is constructor. And that property is standard way to keep it around.

Agree. Although we declare that stamp is a factory. However, factory and constructor in JS is almost the same thing.

markokr commented 8 years ago

I don't completely agree with eric's comment as the only flexibility he's planning for is allowing more args to initializers...

But this is not the main point here. Main simplification I want to push here is:

So if you want to push for single-arg stamps in spec's version 1.0, then initializer signature should be:

init: function (options) { this.foo = options.foo; }

This leaves open both paths: the simple extension (pass arguments) or going with different API like current initializers.

troutowicz commented 8 years ago

There is no need to support fat-arrow functions as initializers as there is better ES6 class-method syntax available for that: { init(){} }

Mmm, can you explain this? Fat-arrow functions and enhanced object method properties are quite different things.

The stamp factory function will take it's arguments and pass them to initializers directly. No need to extra values.

I agree with this.

There is no need for magic keys in options or magic extra args that are specific to stamp launcher. All stamp args belong to initializers.

Agreed here too. Modifiable and Composable should mean different things.

markokr commented 8 years ago

Fat-arrow binds this. In #31 @koresar argued that this means alternative way to pass instance is needed.

troutowicz commented 8 years ago

I understand @koresar's argument for fat-arrow functions and using the instance property instead of the lexically binded this. I don't understand the comparison you are making with enhanced object method properties ({ init(){} }). The initializers descriptor property is an array, not an object.

markokr commented 8 years ago

Instead { init: (opts) => { ... } } people can always write { init(opts) { ... } } so no need to support => functions by having alternative path for instance.

troutowicz commented 8 years ago

You may not have caught my edit of my last comment, but the initializers descriptor property is an array, not an object.

markokr commented 8 years ago

Final descriptor property yes, but when defining single stamp, surely there will be way to use single function or object as input?

ericelliott commented 8 years ago

Initializers signature is same as stamp's, but with this set to new instance.

Stamps and initializers serve different purposes. I don't understand why you think it's desirable to have the same function signature.

ericelliott commented 8 years ago

I think we get the gist of the stamp argument consensus: No reserved keys.

Here's a PR.

I strongly disagree that we should do the same with initializer arguments. I believe that if people want to use this-free code, we should leave that to the author's discretion. It is also useful to retain reference to the original arguments passed into initializers.

I think we should err on the side of developer freedom in these spec discussions.

So, let's discuss the PR, get it merged, and get these threads closed. =)

markokr commented 8 years ago

I see you made options recommended and not required and allow more args. Good!

About stamp/initializer args - I saw unnecessary complexity there and recommended to remove it. They don't need to use same signature, but they can, so it seemed simplify things for developer.

You say you want to give developer freedom and give way to do things multiple ways. I prefer minimal complexity and clarity and want to give only one simple way to do things.

Matter of taste I guess...

ericelliott commented 8 years ago

They don't need to use same signature, but they can, so it seemed simplify things for developer.

No, they can't. If we don't pass anything to the initializers, they won't have access to things that initializers frequently need, such as the stamp descriptor itself.

I saw unnecessary complexity there and recommended to remove it.

In this case, removing it would make it more complicated to do common things, not less complicated. I would rather remove the this binding than remove the instance from the initializer params, but that would confuse some users.

markokr commented 8 years ago

As I mentioned in first message, stamp will be available as this.constructor

ericelliott commented 8 years ago

As I mentioned in first message, stamp will be available as this.constructor

As I just mentioned:

I would rather remove the this binding than remove the instance from the initializer params, but that would confuse some users.

Passing the instance in allows you to use arrow functions without worrying about breaking this, and allows devs who prefer it to use this-free style. As I mentioned before, removing the parameters makes things more complicated, not less. I would rather not bind this at all than remove the parameters from the function signature.

Also, this is not going to happen, because Stamps are not constructors. They intentionally exhibit different behavior because instanceof is an anti-pattern in JS:

The stamp is visible as instance.constructor. Simplest way to get there is use stamp.prototype as compose.methods. This makes instanceof work also.