search

olivernn / davis.js

RESTful degradable JavaScript routing using pushState
http://davisjs.com
532 stars 58 forks source link

Application stop bug fix #68

Open jona10 opened 11 years ago

jona10 commented 11 years ago

Fixed a problem where the application did not detach the internal events from it's callbacks registry when stopping. This has the effect of calling the callbacks as many time as the application was started/stopped during the lifetime of an application.

jona10 commented 11 years ago

This is a follow up from https://github.com/olivernn/davis.js/issues/55

olivernn commented 11 years ago

Did you mean to close this? Is it no longer needed? It still sounds like a bug that is worthing fixing to me…

jona10 commented 11 years ago

Oh, I must have misclicked. Go ahead and re-open it :)

olivernn commented 11 years ago

I've taken a look at this and I'm not sure what bug this is fixing currently, do you have a sample app that re-creates the issue?

When the app is started it only binds to its internal events if they have not yet been bound to, see here in lib/davis.app.js:197.

The problem with unbinding every event handler is that any handlers that the user has bound will be wiped out when stopping the app, this is probably not what you want to happen.

jona10 commented 11 years ago

The problem come from the fact that I instantiate a new Davis.App() to override the old one. "boundToInternalEvents" is stored in the App's scope while the callback registry is stored in a closure and remains the same throughout the application's life (see https://github.com/olivernn/davis.js/blob/master/lib/davis.event.js#L17).

I wrote a failing test reproducing the problem: https://gist.github.com/4655347

From what I'm reading in your last comment, I understand that a stopped app might be restarted via the start() function without the need to instantiate a new Davis.App() and re-register all the routes. If that is truly the case, then I failed to understand the stop() function's purpose and I would not expect my events or my routes to be wiped either :). Perhaps I could interest you in a "destroy" function that would clean the app and bring it back to an un-configured state?

In my current context, I hold a route registry outside of Davis where routes can be added or removed at any time in the lifetime of the app and are expected to work/not work as soon as inserted/removed. Destroy would stop the app and wipe the slate clean.

I could close that pull request, revert my last commit and start working on one such function for a later pull request.

Any thoughts on that?

jona10 commented 11 years ago

Hi Oliver, Sorry for the long time between commits, finally found some time at work to put into this.

This new commit adds a destroy() function to the App, leaving the stop() function as-is. Destroy() stops the application and unbinds every events (user and internal). This brings the app back into a vanilla state, ready to be re-created anew.

Hope everything is to your liking.