Skip to content
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

Add Context#executeCommand #64

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

creynders
Copy link
Contributor

This is the last one, promised ;)

I added an executeCommand method to the context. It's something I realise I could/would use sometimes. For instance in my current project I have a pretty complex control flow state machiny-thing. Basically it's a controller. The reason why I don't rely on a commands/events mapping is because I find it far more clear to have all of this centralised instead of having to track the flow through a dozen or so commands. Anyway, in my controller there are however sites where I do want to execute a command, since the command centralises some complex update mechanic for instance, so ATM I simply dispatch dedicated events, which are mapped to the relevant commands, but actually would like it more to being able to execute the command directly with

this.context.executeCommand(ComplexUpdateMechanicCommand);

executeCommand accepts a second parameter, named "event", which is an object with eventName and eventData members. E.g.

this.context.executeCommand(ComplexUpdateMechanicCommand, {
    eventName : "update:complex:mechanic",
    eventData: {
        importantStuff: "Holy shizzles!"
    }
} );

@geekdave
Copy link
Contributor

geekdave commented Sep 5, 2014

Would executeCommand only be called from within a Context.initialize() function? No other "actors" should have access to the context object directly.

In the View wiring code:

        // only set a reference to the context on the view if the view
        // is a pre-0.7.0 component that does not use dependency injection. 
        // this will be removed in a future release...
        if (!view.wiring) {
            view.context = context;
            returnValue = context;
        }

And in the command wiring code (should have a similar pre-0.7.0 check):

        commandInstance.context = this;

I'd like to get rid of both of these actually, and perhaps bump to a 1.0 release candidate since this is a breaking change (but paving the way for where we want to go with a pure-DI solution, and opposed to the context-as-a-junk-drawer metaphor)

Anyway, tangent... but some food for thought. :)

@creynders
Copy link
Contributor Author

No other "actors" should have access to the context object directly.

Ummmm? I use context in my commands all the time to (un)wire things.

@geekdave
Copy link
Contributor

geekdave commented Sep 5, 2014

Let's think about another way to do this. Perhaps injecting an
"unwireCommand" method aka the injected "listen" and "dispatch". I want to
avoid the context being available as a global object where you can just
stash whatever values at any time.

On Friday, September 5, 2014, creynders notifications@github.com wrote:

No other "actors" should have access to the context object directly.

Ummmm? I use context in my commands all the time to (un)wire things.

Reply to this email directly or view it on GitHub
#64 (comment)
.

@creynders
Copy link
Contributor Author

I (un)wire all kinds of stuff in my commands, not just commands. How do you do it otherwise?
But, if you're really intent on phasing context injections out, obviously I can manually map context if I want to use it in commands.

@creynders
Copy link
Contributor Author

I think having the DI is already discouraging enough to not stash all kinds of stuff in your context? I mean there's no good reason to do so if you have DI right?

@creynders
Copy link
Contributor Author

Anyway, this shouldn't prevent you from releasing, there's no rush to this whatever the outcome.

@geekdave
Copy link
Contributor

geekdave commented Sep 5, 2014

I guess I've never really unwired anything before so maybe I'm missing out
on an entire geppetto chapter!

Generally everything I wire up will stay wired up until the root view is
removed and the entire context is unwired.

What's the use case for things outside the context unwiring other things?
This seems to break the "context as the central nervous system" metaphor
for me, if a nerve in your foot can reach into the central nervous system
and turn off a nerve in your hand.

I would rather the unwiring happen in the context itself somehow.

On Friday, September 5, 2014, creynders notifications@github.com wrote:

Anyway, this shouldn't prevent you from releasing, there's no rush to this
whatever the outcome.

Reply to this email directly or view it on GitHub
#64 (comment)
.

@creynders
Copy link
Contributor Author

For instance it's very easy to implement undo/redo functionality with "dynamic" wiring. I create an UndoModel which has a map of events to do_Command/undo_Command pairs. It wires all of those events to the do_Commands and listens for these events itself. If such an event is triggered it wires the relevant undo_Command to a "undomodel:undo:[index]" event. (The index keeps track of the "stack" size). If it receives an "undo:requested" it dispatches the "undomodel:undo:[index]" event thereby triggering that specific undo_Command and subsequently unwires the undo_Command and decrements the index.

Obviously, come to think of it, would the context have an executeCommand method, I could skip the extra event and simply execute the command directly :)

Another use case would be to have commands wired/unwired to a specific event depending on model state for instance. E.g.: you have a debugging settings panel with a "log remotely" checkbox. If it's ticked a command which invokes the LogService is wired to (an) event(s). When it's toggled off the command is unwired again.

Another advantage is not at run-time but during development, I group some setting up of wirings into a BootstrapDebuggingCommand for instance. The context wires the BootstrapDebuggingCommand to a "startup" event of some sorts, but only if config.debug=true. I suppose you could do this with a separate child context as well..? Haven't tried such a workflow yet, though.

Also, how do you avoid having monolithic context classes?

@geekdave
Copy link
Contributor

geekdave commented Sep 5, 2014

Hmm, I see the use cases but I just wonder if there's a safer way to do this than passing around the whole context object directly. The whole point of the injected dispatch and listen methods were to avoid needing to have a handle to the context itself to communicate via the event bus.

With regard to "monolithic" context classes, I split out my command/view/class/value wirings into separate files, and require them in separately. See: #35

This still could be considered "monolithic" in the sense that the commands.js file knows about all the command wirings. But I would argue this is a good thing. When you want to understand the state of the switchboard (what is wired to what) you only have to look in one place.

With regard to dynamic wiring (one event could conditionally map to one of many commands) I generally try to avoid this, preferring a single command with a conditional. If the business logic in each conditional block starts to become big, you can extract each block out into its own separate utility file, which can be require'd in by the command, itself.

I realize we're probably coming to a difference that's more philosophical (religious?) about how Geppetto apps should be structured. But when it comes to wiring, I tend to favor a structure that's more monolithic (and predictable!), over something that's more dynamic (and hard to follow).

@creynders
Copy link
Contributor Author

With regard to dynamic wiring (one event could conditionally map to one of many commands) I generally try to avoid this, preferring a single command with a conditional.

I'll whole heartedly confess dynamic wiring has bitten me in the ass before. But I think it's a matter of knowing where to use it and when. (As with everything regarding programming 😁)

However I find the monolithic wiring exactly the opposite of clear, it confuses the hell out of me.
Again, I'm a big fan of state machiny flow controllers which have all freedom to wire and unwire whatever is necessary, execute relevant commands et cetera since they centralise control logic. Most of the times an app will have several of such flows in parallel and it's really very clear and readable to have each flow in its own controller not muddied by unrelated wirings.
I have an example of such a flow controller, but this one doesn't do any wiring on the fly, since it's not necessary, however it does show how clearly you can follow what happens when and why. Since this is still WIP it even contains business logic, but that's bound to get extracted out of there into commands. And again, I'd have the necessity to be able to access the context from the flow controller, since either if executeCommand exists I'll be executing the commands directly, or if it doesn't exist I'll be wiring those commands to the events inside the flow controller. Why? It would defeat the purpose of centralising everything into a flow controller, yet still need to wire the commands triggered by the flow controller's events some place else.

But yeah, this is definitely one of those more ideological discussions. I'm not debating this for debate's sake, but to learn whether you solve this maybe more elegantly. ATM I can't see how a monolithic (even when split up) context could be any more clear than such a flow controller.

Anyway, I agree one should try and shield people from themselves, but it shouldn't come at the cost of not facilitating different approaches. As long as we're not giving examples of hanging all kinds of crap on the context IMO we've done our duty.

@geekdave
Copy link
Contributor

geekdave commented Sep 8, 2014

@creynders I see your use cases, but I'd still strongly prefer that we inject only the whitelisted methods we want into Geppetto actors.

We already have this precedent with the injected listen and dispatch methods, so why not inject wireCommand, unwireCommand, executeCommand, etc? If we're providing raw access to the context, then we should just prescribe using this.context.dispatch instead of this.dispatch (which I don't want to do! :) )

I see your point about giving good examples, etc. but at the end of the day, I've still seen developers taking a "quick and dirty" approach of just stuffing some property directly onto the context instead of properly setting it up for injection.

@creynders
Copy link
Contributor Author

Yeah, I understand. I just wonder whether the benefit of the encapsulation outweighs the pollution of the commands. To me dispatch and listen are accepted exceptions since they're injected everywhere.

So, which methods would be considered for injection? The full resolver API: wireSingleton, wireClass, wireValue, executeCommand, resolve, hasWiring, getObject, instantiate, release, configure,
and the full Context API: wireCommand, wireCommands, wireView, dispatchToParent, dispatchToParents, dispatchGlobally.

Seems a bit much to me, yet all of those methods need to be accessible from commands IMO.

@creynders
Copy link
Contributor Author

To sum it up, I think the major difference in ideology between us is that to me commands are the glue that make up the application. All the other parts should be reusable and easily portable, however the application needs to be "made" somewhere: the commands. This means they have to be able to do everything what's necessary. The reason why this is not the contexts responsibility is because the contexts role is exactly that: providing a context into which the components are assembled. Two contexts allow two identical components to live side-by-side at the same time and behave differently. And following SRP (obviously this is debatable) I'd say contexts provide context and commands execute, i.e. do stuff.
Also, I agree that the context should be shielded off. It shouldn't be accessible from anywhere, except the commands.

To me it also goes against best practice to have a class which exposes a ton of facilitating methods (i.e. the context) only to have it operate on itself. It gives too much responsibility to the context and muddies intent. It would be like letting a model listen to all system events directly (i.e. having a contextEvents mapping declaration inside the model class itself) and translating those to API calls on it self.

Then, as a pragmatic programmer I definitely think ease-of-use, i.e. user friendliness trumps strictness every time. Even if hiding the context out of reach of commands would be more correct, the ease-of-use of having it available in them wins out.

On a final note: if the context is only available in commands, even if people start hanging all kinds of crap on it, they can't access it from anywhere else so it would be of very little use to them.

And last but not least: having the context injected into commands allows people to still use it as you do. But not injecting the context into commands will seriously hinder people following my approach: they can't use shallow commands, have to wire the context manually each time, wire many many commands to receive the context, i.e. they'll have to write a lot more boiler plate.

@creynders
Copy link
Contributor Author

And come to think of it there's an alternative option: we could make the context instance non-dynamic, i.e. prohibit to create members that aren't predefined. I'll rather spent my time on coming up with a solution for that, than rewriting Geppetto (which would be necessary) to be able to inject all context and resolver methods into commands.

For instance we could use Object.freeze. Unfortunately it doesn't have a polyfill other than making sure Object.freeze doesn't throw an error in legacy browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants