Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

Don't route requests immediately after loading routes #467

Closed

Conversation

iangreenleaf
Copy link
Contributor

We ran into trouble trying to use the "url" view helper in Brunch for named routes in our layout. This helper depends on the router (obviously), but the layout is being initialized in @initLayout(), which comes before @initRouter in the initialization sequence.

I think the best solution is to load routes at the beginning of application init, making them available to the rest of the init process. Then we call startHistory() at the end to actually route requests.

This will require an adjustment to the Chaplin init sequence. With this patch, our application.coffee file in Brunch now looks like this:

  initialize: ->
    super

    # Register all routes
    @initRouter routes

    # Initialize core components
    @initDispatcher controllerSuffix: '-controller'
    @initLayout()
    @initMediator()

    # Application-specific scaffold
    @initControllers()

    # Actually start routing
    @router.startHistory()

    # Freeze the application instance to prevent further changes
    Object.freeze? this

This allows us to load routes at the beginning of application init,
making them available to the rest of the init process. Then we call
`startHistory()` at the end to actually route requests.
@paulmillr
Copy link
Contributor

Sounds sort-of-reasonable. Although, tests are failin’.

edit: fixed

@iangreenleaf
Copy link
Contributor Author

Yeah, forgot about those, just pushed a new commit.

If you think this is a good way to handle it, Brunch probably needs an update to the boilerplate.

iangreenleaf added a commit to iangreenleaf/brunch-with-chaplin that referenced this pull request Mar 8, 2013
iangreenleaf added a commit to iangreenleaf/brunch-with-chaplin that referenced this pull request Mar 8, 2013
@mehcode
Copy link
Contributor

mehcode commented Mar 19, 2013

Not fond of this. The user shouldn't have to access @router directly unless extending it or something (not in the base use case).

I wouldn't mind if you placed @router.startHistory in the constructor after initialize and have an autoStartHistory property on the application prototype that defaults to true.

@iangreenleaf
Copy link
Contributor Author

@mehcode I'm not sure which constructor you mean.

We could also add a @startHistory method to Application instead. This would match the rest of the init sequence calls a little more closely and avoid direct access of @router.

@mehcode
Copy link
Contributor

mehcode commented Mar 19, 2013

Oh right... the application doesn't call initialize implicitly. Well... should we do something like:

class Application
  constructor: (options) ->
    @initialize options
    # Throw error messages if all components aren't started perhaps?
    @router.startHistory()

Any reason not to @paulmillr / @molily ? I like the sanity checking it provides if someone forgets to init the composer or something.

@paulmillr
Copy link
Contributor

I had async initialize in at least two apps. E.g. wait until some global mediator models / collections are fetched from server.

@mehcode
Copy link
Contributor

mehcode commented Mar 19, 2013

That logic probably belongs in the application under initMediator and we could have a flag like autoStartHistory which could be set to false (it would be true by default) and then startHistory could be called by whatever event.

@paulmillr
Copy link
Contributor

I think we can get consensus on this by adding @startRouting() or @startHistory() method to Application.

@mehcode ?

@mehcode
Copy link
Contributor

mehcode commented Mar 25, 2013

I think we can get consensus on this by adding @startRouting() or @startHistory() method to Application.

Agreed. My votes goes for @startRouting. But I'd be fine with either.

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

Successfully merging this pull request may close these issues.

None yet

3 participants