-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
allow "instance"-style usage #17
base: master
Are you sure you want to change the base?
Conversation
init: function init (context) { | ||
var target = _.first(context.args) || this; | ||
|
||
_.defaults(target, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole thing will prioritize the configuration in the first parameter, but if it's not passed, then it'll try to take it from the second parameter, and if it's not there either, just use the defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the defaults are defined in props
, but if target
is not this
, then they aren't going to be used, which is why we have to do it twice. Unless there's a really good reason to support the second parameter, maybe we should drop that functionality, because it's a bit cumbersome. In other words--we can now mix any object into a state machine in addition to mixing a state machine into any object, but do we need both?
Once you've considered the ideas herein I'll add proper tests, if you're to go that way. |
In my view the configuration object (or the first parameter) is the specification or blueprint of the state machine. Among the properties of the configuration, the most important is the These are the basic features of the configuration, so I don't quite understand why there are more properties in the configuration if they are not strictly related to the behaviour of the state machine (e.g. If there is some other data that used by the state machine instance it can be provided in a closure or as the second parameter. Stampit is new to me so I might have missed some of the point you made... On the other hand, this PR is quite large and it might be split into smaller pieces, or just itemise the features that it brings in, such as mixing event emitter into the instance if that does not have it, configure the promise lib via |
I'll try to explain Stampit as best as I can below. The most basic Stamp (a composable factory function), is the following: // Ex.1
const MyStamp = stampit();
const myObj = MyStamp(); // alternatively, MyStamp({})
myObj; // {} Example 1 above is functionally equivalent to: // Ex.2
const myObj = Object.create({});
myObj; // {} Both examples 1 and 2 are trivial and useless. If we want to make a factory function ourselves which actually does something, we could write: // Ex.3
const prototype = {
foo() {
return this.value;
}
};
function factory (instance) {
return Object.assign(Object.create(prototype), instance);
}
const myObj = factory({value: 'bar'});
myObj.foo(); // bar Stampit provides the ability to write example 3 as: // Ex.4
const MyStamp = stampit({
methods: {
foo() {
return this.value;
}
}
});
const myObj = MyStamp({value: 'bar'});
myObj.foo(); // bar In a nutshell, Stampit provides more bells and whistles around example 3. What is common between these two implementations is that the first parameter to the factory function is "mixed-in" to the resulting object--the resulting object is never strictly equal to the first param passed into each function. So then, your comment above:
...poses a problem, because it is directly at odds with Stampit's API, if I take it to mean what I think you mean. I have some questions, then What is a "state machine"? Assuming the object returned is by the function a "state machine" (even though it may really be just a proxy):
I didn't answer the last two, because I don't know. I do know that providing a function which may or may not have side-effects is not the most straightforward API. If I am going to make an argument, it's that:
To your argument, I see where you're coming from--the properties passed as the first parameter should "configure" the resulting object, but not be properties of it. To make this work with the Stampit API, the most straightforward solution would be to pass the config as the second parameter, leaving the first for any properties which are to be mixed-in to the returned object (you see the first parameter as the "configuration", whereas I'm coming from a place where that first parameter contains object properties). OK, so that's a solution, but it's not very elegant, nor is it user-friendly if you don't have any extra parameters! An API such as this may be easier to stomach, however (and this is trivially implemented, mind you--it mimics my own wrapper of // Ex.5
const TrafficLight = StateMachine
.methods({
slamBrakes() {
// stuff, returns Promise
},
speedUp() {
// other stuff, returns Promise
}
})
.event({
name: 'go',
to: 'green'
})
.event({
name: 'slowDown',
to: 'yellow',
from: 'green'
})
.event({
name: 'stop',
to: 'red',
from: 'yellow'
})
.initial('go')
.final('stop')
.callback('enterYellow', function () {
return this.floorIt();
})
.callback('leaveYellow', function () {
return this.slamBrakes();
});
const lightA = TrafficLight({name: 'A'});
const lightB = TrafficLight({name: 'B'});
const lightNoName = TrafficLight(); Above, Using an API like this--even supporting the config-as-second-parameter idea--we can avoid exposing the state machine's guts to the user. Either way, the API must change if you are to fully leverage Stampit's power and flexibility; otherwise if I want to write a plugin or fiddle with something, I'm back to monkeypatching. Please let me know what you think before I modify the PR further. |
Yes, I agree with both statements. The API in example 5 is a good compromise that addresses my concerns and also plays nice with Stampit. I like a lot the ability to create the instance independent state machine factory, which can be used to create the actual instances that can be configured via the provided object. I have use cases in which I provide only the configuration so it would be a bit odd to have an API in which the first parameter is I would assume that the state machine factory function can be created using the chaining as in ex 5 but also by providing the "classic" configuration object from the current API. I assume that the |
Sounds great. I'll modify this accordingly with the proposed API. |
This needs tests, obviously, but it brings fsm-as-promised more inline with how these composable factories are intended to be used. PR #15 was simply a refactor; almost no actual API changes were involved.
Whenever you call a "stamp" (a composable factory function) created by Stampit, the first parameter is always going to be an object. The object returned is, by default, not the object you pass as the parameter. However, the object you pass (henceforth known as an "instance") is mixed in (via
Object.assign()
,_.assign()
, etc) to the resulting object, e.g.:fsm-as-promised's API allows mixing in of an instance via the second parameter; the first, of course, being simply the configuration.
With these changes, we can now compose freely with
fsm-as-promised
. Before this change, the following would not work as expected:Above, the first parameter is used for configuration, then thrown away. To get this behavior, you'd need to do:
Now, the above is still supported, but the problem with this is that if you ever wanted to compose from
StateMachine
, you're in for pain, because the first parameter is thrown away.Oops. Anyway, the old behavior is still supported with some trickery--and it will now "mix in" an EventEmitter into the second parameter (
target
) if it has noemit
function--but now this works as expected:This is accomplished by providing a second factory function (
Target
) which creates an instance ofStateMachine
internally, which sets everything up.StateMachine
then exposes amixin
object for theTarget
to mix into itself via_.assign()
.Of note, when you write:
you are now no longer using the
StateMachine
factory; you're actually gettingTarget
. For hacking's sake, to get the actualStateMachine
factory, you'd do this:This change should be transparent to the user, unless they are vehemently opposed to having an
EventEmitter
mixed in to their target object (second parameter, as per the old API).EventEmittable
is a composable factory function (a "stamp") wrappingEventEmitter
. To use it, simply call.compose(EventEmittable)
upon any stamp.