stampit-org / stamp-specification

The Stamp Specification: Composables
434 stars 16 forks source link

The new shared "protected" variable inside intializers #84

Closed koresar closed 8 years ago

koresar commented 8 years ago

Previously, the only way to share data across stamps were the public properties. From time to time it is necessary to share data between stamp privately.

This is easy to achieve with one more variable passed to each initializer - shared. It should be a simple object {}.

const SetSecretKey = compose({
  initializers: [({ key }, { shared }) => {
    shared.secretKey = key;
  }]
});

const SecureConnect = compose({
  initializers: [(options, { shared }) => {
    this.connect = () => connectToSecureCloud({ secretKey: shared.secretKey });
  }]
});

const SecureCloud = compose(SetSecretKey, SecureConnect);

const connection = SecureCloud('my secret').connect();

/cc @ericelliott @BauchBeinePoe Original idea: https://github.com/stampit-org/stampit/issues/199

koresar commented 8 years ago

"protected" is a reserved word. We better not use it. Babel does not compile it. :) How about "shared"?

unstoppablecarl commented 8 years ago

How about internal?

koresar commented 8 years ago

Sounds good. But I prefer "shared". :)

unstoppablecarl commented 8 years ago

so this object can only be accessed or mutated within an initializer function?

I am not sure how I feel about this. I read stampit-org/stampit#199 . What is a use case could only be accomplished with this? It seems to be more a flavor preference, not a new feature.

koresar commented 8 years ago

@unstoppablecarl I've just updated the code in the initial comment. Please, take a look. Does it answer your questions?

JosephClay commented 8 years ago

Is the shared object protected in some way? Otherwise I'm failing to see how this is different than attaching secretKey to the instance.

stefanpoeter commented 8 years ago

This way the 'secretKey' is not part of the public API of that specific stamp.

ericelliott commented 8 years ago

This is an anti-pattern. Stamps should not know about each other's internal state -- let alone share it.

This is how tight coupling happens in classical inheritance.

koresar commented 8 years ago

The only safe way to achieve that "shared" state is to wrap Stamps into another factory function. I hated the approach of wrapping factory function into factory function.

I needed that feature from time to time when I wanted to split my huge stamp onto 2-5 smaller stamps. Same time I needed them to have access to the same private state. I had to wrap those factory functions into a another factory function.

Some times you deliberately want to tight couple stamps.

Didn't you ever had this before guys?

koresar commented 8 years ago

Moreover, in my practice, having no "shared" state sometimes does the reverse job - you have to couple non related code into the same initializer! (I can provide real world code example if you ask.)

ericelliott commented 8 years ago

No, I never deliberately want to tightly couple stamps -- it runs directly counter to the whole purpose of stamps, which is to avoid this sort of tight coupling that is forced on you if you use classical inheritance. You might as well start inheriting from a base class if you're going to tie your stamps together like this.

If you think you need to do this, perhaps what you really need is to split one stamp into several modules, and share the state internally in one stamp that wraps the modules together.

Stamps are a nice abstraction, but stamps are not the ONLY abstraction you should make use of.

koresar commented 8 years ago

split one stamp into several modules,

That's exactly what I need and did.

and share the state internally in one stamp that wraps the modules together.

That is exactly what I want to avoid very-very much - wrapping factories into factories. First of all, it's more code to look after. Second, it tight couples the wrapper to those several modules.

I think we need to provide code in order to have productive discussion.

Eric, if you have cycles to code what you mean, then please provide your solution code. Meanwhile, I'll prepare my real world example.

ericelliott commented 8 years ago

I don't have time right now, but that doesn't mean that tightly coupling stamps with shared private state is a good idea. This is not a "person with the most free time wins" argument. It speaks to the core philosophy of stamps, and why they exist in the first place.

koresar commented 8 years ago

I want stamps to get used world wide instead of classes. I want stamps to win.

Why JavaScript is the most used language in the world? Because JavaScript is freedom. The language have all the features of other languages.

If we introduce "shared" state this will mean more freedom.

That's my philosophy. :)

(Preparing few LOC now. Might even prepare the code you meant two comment above.)

stefanpoeter commented 8 years ago

I want stamps to get used world wide instead of classes. I want stamps to win.

Sounds like the wrong motivation!

I like to use stamps because of the ability to compose modules. I favour composition over inheritance. But even when I compose small modules I still make assumtions in these modules what I understand is tight coupling. Please correct me if I am wrong. Why should I even compose objects that do not share a state with each other and, if this state is of no concern to the public, why should this be visible to the public?

I am really curious for an example. Maybe I didn't get the idea of stamps right?!

unstoppablecarl commented 8 years ago

Stamps are a nice abstraction, but stamps are not the ONLY abstraction you should make use of.

Agreed. Just because you cannot use a stamp to accomplish your use case does not necessarily mean it needs a new feature to accommodate it. If you have a case where you need to tightly couple stamps, they should probably be wrapped into one module in some way. The point and definition of as stamp is an independent, encapsulated, uncoupled factory. Adding a feature with the single purpose of easing coupling is an unwise choice.

@BauchBeinePoe There is a difference between stamps that reference the same properties / methods and tight coupling. Anything shared between stamps / modules is not internal / private state it is accessible outside of the module scope and is public.

If you can provide a use case I am sure we can explain how this applies in more detail. You should join our Gitter channel.

stefanpoeter commented 8 years ago

A simple use case would be a simple tcp client with a request/response or rpc pattern.

The client offers methods a, b and c that make requests through a tcp socket. I would implement the tcp socket and implement every method in a different stamp. For the client module i would compose the stamps for a, b and c and the stamp that handles the socket.

var socketStamp = stampit().init(function () {
        this.socket = ...
    });
var aStamp = stampit().init(function () {
    this.a = function () { 
        // do something with this.socket
    };
});
var bStamp = stampit().init(function () {
    this.b = function () {             
        // do something with this.socket
    };
});
var cStamp = stampit().init(function () {
    this.c = function () { 
        // do something with this.socket
    };
});

module.exports = stamp().compose(socketStamp, aStamp, bStamp, cStamp);

a, b and c are not coupled to each other but rely on the socketStamp. I can add and remove functionality by writing new stamps like dStamp etc. The socket is exposed in the public API.

ericelliott commented 8 years ago

This. 1000 times, this. Burn it into your brain.

The point and definition of as stamp is an independent, encapsulated, uncoupled factory. Adding a feature with the single purpose of easing coupling is an unwise choice.

ericelliott commented 8 years ago

Remember, you don't have to do everything with stamps. Other patterns that work really well with stamps:

For example, you could pass the socket into the stamp:

var aStamp = stampit().init(function ({ socket }) {
    this.a = function () { 
        // do something with socket
    };
});
var bStamp = stampit().init(function ({ socket }) {
    this.b = function () {             
        // do something with socket
    };
});
var cStamp = stampit().init(function ({ socket }) {
    this.c = function () { 
        // do something with socket
    };
});

// pass socket in from outside
module.exports = stamp().compose(aStamp, bStamp, cStamp);

Is-a vs uses-a

Note how the first socket example makes the socket part of the object being used, so much so that it's integrated by virtue of all the other stamps depending on it to be there, attached to this.

In other words, the object is a socket-object.

In the dependency injection example, each stamp gets handed the socket to use, creating a has-a/uses-a relationship (note the lack of this: a key differentiator in this case).

ericelliott commented 8 years ago

I want stamps to get used world wide instead of classes. I want stamps to win.

How will stamps win if we allow them to become yet another way to create tightly coupled class inheritance hierarchies?

ericelliott commented 8 years ago

Everybody, please carefully read "What's the Difference Between Class and Prototypal Inheritance" before commenting further in this thread.

Along with this and this if you're struggling to understand why the coupling being proposed will get you into the same kind of trouble you get into with class inheritance.

The shared state creates coupling similar to the way that calling super() creates coupling -- but it does more than that -- it also opens the door for concurrency bugs such as race conditions -- particularly when you're talking about networking... For more on that topic, see The Two Pillars of JavaScript Part 2.

I get it. We love stamps... but please don't use stamps for absolutely everything. There's a time and a place for stamps, and there's a time and a place for functional programming, modules, and dependency injection. Instead of trying to stuff a round peg into a square hole, think outside the stamp.

unstoppablecarl commented 8 years ago

@BauchBeinePoe I think the way you have written that example is acceptable in most cases. As written you have not necessarily tightly coupled the definition of the socket object to external elements depending on it. An object property ( obj.socket ) can describe a uses-a relationship.

You have created a dependency on an easily swappable property with the name socket ( this.socket ) with the requirement that it matches an expected api interface. This does not tightly couple setting of the socket property to any code outside the stamp.

Personally, I do not bother creating variable privacy for stamps ( or in general really). They are much easier to understand and debug when properties are accessible.

ericelliott commented 8 years ago

You have created a dependency on an easily swappable property with the name socket ( this.socket ) with the requirement that it matches an expected api interface.

I mostly agree, but it's a slippery slope. Most stamps should not be aware of other stamps, or make assumptions about the shape of the instance object if that shape is outside the control of the stamp.

If you start sharing API between stamps, consider dependency injection. You can create a wrapper stamp which knows about the dependencies and passes them into dumb stamps, which have no awareness of anything other than themselves and their parameters.

A wrapper stamp can return a wrapped stamp and use dependency injection internally.

unstoppablecarl commented 8 years ago

@ericelliott When would the shape of the instance object be outside the control of a stamp?

@BauchBeinePoe This is how I would write your use case:

// used for reference and testing
var socketStub = stampit()
    .props({
        // describe the shape / interface of the socket object
        socket: {
            foo: function() {},
            bar: function() {}
        },
    });

var aStamp = stampit()
    // dependency
    // .compose(socketStub)
    .methods({
        a: function() {
            // do something with this.socket
        }
    });

var bStamp = stampit()
    // dependency
    // .compose(socketStub)
    .methods({
        b: function() {
            // do something with this.socket
        }
    });

var prodStamp = stampit().compose(aStamp, bStamp);
var prodInstance = prodStamp({socket: appSocket});

I keep // .compose(socketStub) as a reminder of the dependency. .compose(socketStub) tells me exactly what it needs. In my app I pass in the socket as an argument.

If a stamp needs an instance of something it should probably be set by passing it to the factory function. Ideally, you are only composing things before creating anything you would consider a "runtime" object instance. In js there is no separate compile and "runtime" but it is still a good idea to do one then the other as much as you can. When the code is composing stamps the value of socket is to be determined, not composed in.

A dependency injection mechanism would determine the value of appSocket before passing it to the factory.

stefanpoeter commented 8 years ago

I really do not see the benefit of this. All examples have dependencies the one way or another. I actually do not care if it is a is-a or uses-a relationship, that's just another pair of shoes in my eyes. All examples won't work without a socket with a specific API. Sure it is good to have independent, encapsulated, uncoupled factories for many reasons. But if that means that I have to wrap everything together and write proxies then I do not see the point here. And sure, I can use other tools.

But to bring this to closure. If the stamps are intended to be, what @ericelliott says

The point and definition of a stamp is an independent, encapsulated, uncoupled factory. Adding a feature with the single purpose of easing coupling is an unwise choice.

Then a shared/protected state is not the way to do it and shouldn't be in here.

koresar commented 8 years ago

Stamps are designed to ease developers' life.

I feel rather different and do not agree with this. It's not my point. It is yours.

The point and definition of as stamp is an independent, encapsulated, uncoupled factory.

This is how I feel about @ericelliott arguments above:

All examples have dependencies the one way or another.

For example, coupling two stamps over a public property (and this is available since stampit v0.1.0):

const KeyState = compose({initializers: [function({key}) {
  this.key = key;
}]});
const Stamp1 = compose({methods: {stamp1Method() {
  console.log(this.key); // coupling state
}}});
const Stamp2 = compose({methods: {stamp2Method() {
  console.log(this.key); // coupling state
}}});

As you can see stamps are not protected against coupling. And never were. There are multiple other ways to couple stamps. Do you think limiting stamp users freedom will benefit them? We have to give people a choice.

If you think that wrapping stamps into a stamp/function would decrease coupling - then you're not looking far enough. Wrapping stamps/functions into other stamps/functions is simply shifting coupling to a different place.

Although, I understand your concern. You are afraid that this new feature would mitigate coupling, that stamp users would unwillingly couple their stamps. I do agree.

We can find an acceptable compromise. That's why we all love open source software in the first place. :)

Proposal: How about we warn users that by using this feature they will couple stamps?

For example instead of "shared" we could name the property "coupled", or "coupledState", or "sharedCoupling", etc.

This would warn novice users, and give necessary powers to the advanced.

JosephClay commented 8 years ago

I still think this is weird. To me, it's not about the coupling as much as it's an attempt at privacy.

Dumping data into separate, parameter object and passing it around because you don't want a key/value set to a different, parameter object doesn't make sense. There is no privacy. The next stamp can copy the entire shared object to the instance.

If we don't want this.key because it would be public, then don't:

const SetSecretKey = compose({
  initializers: [() => {}]
});

const SecureConnect = compose({
  initializers: [({ key }) => {
    this.connect = () => connectToSecureCloud({ key });
  }]
});

const SecureCloud = compose(SetSecretKey, SecureConnect);
const connection = SecureCloud('my secret').connect();

No need for a secret object. Is it coupled? Maybe, but less so than passing around a secondary object.

koresar commented 8 years ago

@JosephClay @unstoppablecarl @BauchBeinePoe @ericelliott your opinions matter. Seems like we are leaning towards 3 vs 2.

I assume decision is made - no shared/protected state. The relevant PR is declined. In case someday we'd need to introduce the similar functionality - it could be done easily in backwards compatible way.

Thank you very much guys for the productive discussion.