Replace close operation with dispose

[ccamarat]

This is a breaking change request.

The close operation acts as a destructor for Marionette Views, and destructed objects should not be reused. The name close doesn't convey this intention and Marionette's users regularly attempt to reuse closed views. Seeing unusual behavior, attempts have been made to patch individual bugs without fully realizing the scope of the problem. Marionette 1.0.3 contained the first of these partial bug-fixes, but it failed to account for all the things that the various types of views do during their construction that would need to be re-done if the view was to be reused, and so the problems persisted. Other pull requests attempt to resolve various problems arising from this usage, but none have fully addressed the problem, and I'm of the opinion we shouldn't try.

I submit that closeing s view should truly prep it for garbage collection. To clearly convey the intent of the function I propose that it be renamed to dispose instead.

Additionally, since there seems to be a genuine need for swapping views without disposing them, this pull request has added a swap method to the Region.

[samccone]

Hey @ccamarat great rational with this, I wonder if instead of dispose we use something similar to what backbone has with models

destroy

[cobbweb]

Props for such a comprehensive PR! I agree with @samccone and would think 'destroy' would be better (it's more obvious that its destructive and already used in Backbone).

[ccamarat]

Thanks guys. Sorry for merging in others' changes; still getting the hang of GitHub. I will undo that, rename dispose to destroy for consistency with Backbone and take @samccone's other line comment advice. It'll take a couple days.

[nileshkale]

+1 for destroy()
I hope a migration guide would be very helpful giving use cases where changes need to be applied to keep the code efficient for apps that swap views in & out. A simple replacement of close() with destroy() may not be completely helpful.

Better still would be a pattern/use case explaining the right way of swapping views.

Regards

Nilesh Kale

Sent from my iPad

On 16-Sep-2013, at 23:10, "Chris Camaratta" notifications@github.com wrote:

Thanks guys. Sorry for merging in your changes; still getting the hang of GitHub. I will undo that, rename dispose to destroy for consistency with Backbone and take @samccone's other line comment advice. It'll take a couple days.

—
Reply to this email directly or view it on GitHub.

[ccamarat]

Ok, I've reverted the changes that shouldn't have been in there in the first place and renamed dispose to destroy. I'm currently working on updating the render operations to throw an exception if called on a destroyed view, as per @samccone's suggestion; expect another couple days.

@nileshkale - Sure, I can do a writeup.

[nileshkale]

+1
I'm planning to rewrite critical bits of code ones the next version with swap functionality is available! Will make the code much simpler!

Thanks.

Regards

Nilesh Kale

Sent from my iPad

On 19-Sep-2013, at 10:39, "Chris Camaratta" notifications@github.com wrote:

Ok, I've reverted the changes that shouldn't have been in there in the first place and renamed dispose to destroy. I'm currently working on updating the render operations to throw an exception if called on a destroyed view, as per @samccone's suggestion; expect another couple days.

@nileshkale - Sure, I can do a writeup.!

—
Reply to this email directly or view it on GitHub.

[ccamarat]

@samccone - I believe all your suggestions are in now. Code review please?

[samccone]

hey @ccamarat can you squash this commit back into your earlier changes so that you are not changing and then rechanging your code within a single PR.

[ccamarat]

Yea, I can do that. Sorry, I'm not too familiar with GitHub etiquette - what's the general practice then, do I close this PR and open a new one with the updates?

[samccone]

nope you can just force push over your branch, the PR will automatically be updated

[ccamarat]

@samccone - Squashed.
@nileshkale - Updated the documentation with proper use cases for destroy and Region.swap

[mbhnyc]

👍 really like the consistency of this.

[samccone]

Hey @ccamarat this is really amazing work great job!

I left a few nits also it seems like you did not add docs for

    Marionette.triggerMethod.call(this, "swap", view);
    Marionette.triggerMethod.call(oldView, "swapped:out", view);
    Marionette.triggerMethod.call(view, "swapped:in");

that is all, then we can bump the version and 🚢

[samccone]

nice, maybe rename this last commit to Add Documentation for Swap Events, so the git log will read better.

[ccamarat]

Oh, sorry Sam. I'm messing with the docs and don't have a local reader. I'll squish it when I'm done. hopefully another 10 mins.

[samccone]

cool I am going to give this my ⭐

When we merge we can bump the major version and note the breaking change of close in the changelog

ping @derickbailey @mbriggs @jsoverson @tgcondor

[samccone]

Hey @ccamarat we are working on the v2.0.0 release here
https://github.com/marionettejs/backbone.marionette/tree/v.2.0.0-dev

I tried and merge and i got tons of merge issues, would you mind opening a PR into this branch?

Thanks!!

[ccamarat]

@samccone Sorry I've been busy moving. Do you still need me to do a PR?

[samccone]

yeah that would be awesome, thanks @ccamarat

[ccamarat]

Ok, gonna get this done this weekend.