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

Better message handling #145

Closed
zephraph opened this issue Jan 12, 2016 · 8 comments
Closed

Better message handling #145

zephraph opened this issue Jan 12, 2016 · 8 comments

Comments

@zephraph
Copy link
Contributor

Currently when one wants to handle messages or events they add something like the following

return {
  onmessage: function(name, data) {
     switch(name) {
        case 'test-message': test(data);
        break;
        // ...
     }
  }
}

Something like that, right? Well, maybe instead of a switch an if else statements is used. Regardless, that breakdown is up to the individual who implements the module/behavior/service in question.

I pulled this straight from the docs:

Don't include application logic in the event handler. Everything the module is capable of doing should be represented as a method on the object and the event handler should call out to those methods.

This interface doesn't naturally support that. On the contrary, I feel like it encourages the bad practice of handling all cases directly inside that method.

I'm proposing an improvement to message/event handling where an object can be passed to on* as an alternative to passing a function.

Example

return {
  onmessage: {
    'test-message': function(name, data) {
     }
  }
}

This interface ensures that there's a function that handles every message instead of a single function that handles all messages. There's no noisy if or switch statements to deal with. The other thing this setup makes me think is, now that it's known what messages are being handled by the T3 construct, it's not necessary to specify messages at all because that'd just be redundant. T3 can just scrape the messages off the onmessage object.

My current proposal isn't to get rid of messages or the fact that onmessage or onclick can be a function. It's really just the addition for something like onmessage to be an object. Though, I will say that given the next "major" release, which might be a while from now, it would be good to consider the former points.

Thoughts?

@donvadicastro
Copy link

Agree.
More readable and cleaner, also no logic inside event messaging. 👍

@j3tan
Copy link
Contributor

j3tan commented Jan 12, 2016

This is pretty nice idea. It would make the messages array defunct since you could just pull the keys off the onmessage property. I don't think it'll solve the problem of having a bloated onmessage property with application logic, but it would definitely reduce if/else statements.

I'll revisit this proposal when we're thinking of doing another major release. Thanks!

@zephraph
Copy link
Contributor Author

I'd encourage you to think about it in as a minor enhancement in the interm. You could support both objects and functions going forward and deprecate the use of direct functions at a later date.

It would help with the upgrade path too, especially for those who have large systems.

@j3tan
Copy link
Contributor

j3tan commented Jan 12, 2016

@zephraph Would you be willing to put together a pull request for this enhancement?

We'd like to make sure its compatible with IE8 (no ES5) and we can forgo removing the messages array for now.

@zephraph
Copy link
Contributor Author

Sure thing.

@zephraph
Copy link
Contributor Author

I was going back through this in prep for the PR and noticed I left off an example of event handling. Delegation of events is a bit trickier because there's not some set string to key off of. You could key off of data-type, but my assumption is that not all events will have a corresponding data-type.

One way to achieve this would be to have a special method on the object that would determine which event handler should be called.

return {
  onclick: {
    delegate: function(event, element, dataType) {
      return dataType || event.target;
    },
    btn: function(event, element, dataType) {} // Where btn is a dataType
  }
}

I'm not sure if I like this.

Ultimately though, I feel like there should between some semblance of consistent behavior between modules and event handlers.

Is there a better way to handle this?

@j3tan
Copy link
Contributor

j3tan commented Jan 20, 2016

onmessage handling and on[event] handling are fairly different. In onmessage, you are only listening to specific messages defined in the messages array. In the onclick case, you might want to do something more generic like logging every click or doing an event.preventDefault(). Let's constrain the PR to the onmessage method for now.

@zephraph zephraph changed the title Better message/event handling Better message handling Jan 25, 2016
@jonathanamartin
Copy link

There might also be a need to add multiple message handlers. Using the on[message] syntax would tie one message to one handler.

An alternative might be to allow object literals in the messages array. If the value in an index of the array is a string invoke onmessage as usual. If the value of an index is an object then the key would be the message name and the value would be a string literal or list of string literals defining associated callbacks.

For example:

messages = [
    'normalmessage',  // Invoke 'onmessage'
   { 'anothermessage': [ 'callbackone', 'callbacktwo' ] }  // Invoke all callbacks for message 'anothermessage'.
]

Currently I've written a service with a dispatchMessage method (using the on[message] strategy). However, it'd be nice if I could just explicitly map callbacks in the messages array.

@zephraph and @j3tan what do you think?

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

No branches or pull requests

4 participants