brunch / brunch-with-chaplin

Boilerplate application for Brunch with Chaplin framework included.
paulmillr.com
280 stars 68 forks source link

Reverse router lookups without events #59

Closed iangreenleaf closed 11 years ago

iangreenleaf commented 11 years ago

When dealing with chaplinjs/chaplin#467, I was convinced for a while that our problems were stemming from this Handlebars helper:

Handlebars.registerHelper 'url', (routeName, params..., options) ->
  url = null
  mediator.publish '!router:reverse', routeName, params, (result) ->
    url = if result
      "/#{result}"
    else
      routeName
  url

That publish has an async call signature, and if it was async, this helper would not work.

It seems like an odd use of an event to me. We're not really triggering an event and waiting for it to be handled -- we're just asking the router to translate a name into a route. Could we have a way to ask the router directly, or would that violate the separation of concerns?

In our app, I refitted the helper like so:

Handlebars.registerHelper 'url', (routeName, params...) ->
  url = window.MyApp.router.reverse routeName, params
  "/#{url}"

Of course, this needs a global reference to the application:

# initialize.coffee
$ ->
  app = window.MyApp = new Application()

Thoughts?

akre54 commented 11 years ago

mediator.publish isn't async -- publish is aliased to Backbone's trigger, and the callback function is still being executed synchronously as it's triggered. If you look in Chaplin's router.coffee, you'll see the reverse handler is doing exactly what your helper is doing under the hood. If you prefer the second pattern, it seems like a perfectly valid use to me.

I would tend to agree with you that this isn't really an "event", per-se, and it seems like a pretty odd pattern for this case. Maybe @mehcode or @paulmillr could shed some light?

mehcode commented 11 years ago

I agree that pattern is a bit confusing.

Could we have a way to ask the router directly, or would that violate the separation of concerns?

It would. I do something like this in my application:

module.exports = class Application extends Chaplin.Application
  # ...
  initMediator: ->
    Chaplin.mediator.reverse = => @router.reverse.apply @router, arguments...
    Chaplin.mediator.seal()

However the new composer uses the publish pattern to get the value of a currently composed composition in https://github.com/chaplinjs/chaplin/blob/master/src/chaplin/controllers/controller.coffee#L36-L42.

I would probably actually call for a helper that wraps the !router:reverse event somewhere that it would be used most frequently. Perhaps in Chaplin.View so one can just do @reverse and not worry about the weirdness.

iangreenleaf commented 11 years ago

@mehcode that example is precisely the kind of idea I've been toying with. If we proxy it through the mediator, we can have a more ordinary function signature, and still have effectively the same amount of separation.

iangreenleaf commented 11 years ago

I'm warming up to the idea of wrapping the event, too. An advantage of that is it could be implemented in Chaplin proper, instead of as an extra thing in Brunch. We'd just need to confirm that the event call is never, ever, ever going to become asynchronous. Maybe I've spent too long in Node land, but calls that take a callback and still behave synchronously make me nervous.

mehcode commented 11 years ago

We'd just need to confirm that the event call is never, ever, ever going to become asynchronous. Maybe I've spent too long in Node land, but calls that take a callback and still behave synchronously make me nervous.

Considering the code https://github.com/documentcloud/backbone/blob/master/backbone.js#L96-L111 and that backbone is 0.9.10 coming up on a 1.0 it is pretty safe to say it will stay synchronous. It has been discussed many times that backbone.event.trigger is synchronous however event listeners are normally asynchronous. In our case we're in control of the event listener and know that is synchronous. This needs to be documented along with whatever helper we include of course.

Want to open an issue / PR in chaplin for discussion? I'm thinking of having the method in Chaplin.View as Chaplin.View#reverse (implemented as a wrapped event publish).

iangreenleaf commented 11 years ago

Sounds good, I'll put something together tomorrow.

andriijas commented 11 years ago

I renamed this helper to url_for in my projects to avoid clashes with variables named url. just to keep my brain sane. funny because i think i was the one who commited the else routeName part...

Handlebars.registerHelper 'url_for', (routeName, params..., options) ->
  url = ''
  mediator.publish '!router:reverse', routeName, params, (result) ->
    url = "/#{result}" if result
  url

I dont have any problem with it being a mediator publish event. Strange? maybe but as mentioned is used with the composer. Its a good way to seperate concerns i think?

paulmillr commented 11 years ago

Comrades, what’s the point of having Chaplin.View#reverse? I want to use template helper, how is it possible to implement template helper call to current view?

Placing reverse on mediator sounds like a better idea for me.

mehcode commented 11 years ago

I meant as a static method on view. But perhaps the mediator is a better place for it anyway.

andriijas commented 11 years ago

If its going to be static why not the router it self

Chaplin.Router.reverse()

On Friday, March 8, 2013, Ryan Leckey wrote:

I meant as a static method on view. But perhaps the mediator is a better place for it anyway.

— Reply to this email directly or view it on GitHubhttps://github.com/paulmillr/brunch-with-chaplin/issues/59#issuecomment-14630232 .

paulmillr commented 11 years ago

because router can be overridden and stuff and we want only one sort-of-global-var in apps: mediator

andriijas commented 11 years ago

That makes sense but for some reason it feels dirty to pollute the mediator with reverse url lookups. because its business logic, not collections/models which you typically put on the mediator like mediator.users or mediator.session

I think the current way is fine.

perhaps you could explain what kind of problems you ran into @iangreenleaf that led up to the events of submitting this issue and chaplinjs/chaplin#467 more than the pattern being confusing? because I dont even understand why the routes needs to be initiated before everything and then start history. :S

mehcode commented 11 years ago

because router can be overridden [...]

Good point. We don't want to static methods for the simple reason that these elements are bound in the application startup sequence. Someone could actually replace the router or override reverse.

That makes sense but for some reason it feels dirty to pollute the mediator with reverse url lookups. because its business logic [...]

That makes sense too... however there really isn't anywhere to put a method like this in chaplin core aside from the mediator. Perhaps in utils? Or another file called helpers?

paulmillr commented 11 years ago

utils +1

akre54 commented 11 years ago

:thumbsup: utils