marionettejs / backbone.marionette

The Backbone Framework
https://marionettejs.com
Other
7.06k stars 1.26k forks source link

{preventDestroy: true} does not seem to work properly... #2046

Closed thisissami closed 9 years ago

thisissami commented 10 years ago

Hi All,

I've been getting into Marionette recently and have begun building the front end system for a platform that I've been hired to create. I've ran into what I presume is a bug with Marionette, and wanted to check in here.

In my app, I have a div with id main. This is set up to be a region for my app. In this region, I show a LayoutView. This layout view contains a region, which contains an ItemView. When this view is clicked, the entire main layout is replaced with a new ItemView, with the {preventDestroy: true} attribute passed in. This new ItemView, when clicked, returns the original LayoutView to the main region.

However, the original LayoutView no longer exists when I bring it back to my main region. Perhaps of interest, it is only destroyed after the click in the second ItemView, when the code to bring back to original LayoutView is ran.

Here is the HTML:

        <div id="main"></div>
        <script type="text/template" id="template1">
            HELLO HELLO HELLO
            <div id="test"></div>
        </script>
        <script type="text/template" id="template2">
            CLICK HERE TO SWITCH VIEWS
        </script>
        <script type="text/template" id="template3">
            CLICK HERE TO GO BACK
        </script>

And here is my JavaScript code that I run:

    var MatchManager = new Marionette.Application();
    MatchManager.addRegions({
        main: '#main',
    });
    window.runTest = function(){
        MatchManager.layoutView = Marionette.LayoutView.extend({
            template: '#template1',
            regions: {
                test: '#test'
            },
            onShow: function(){
                this.test.show(new MatchManager.itemView());
            },
        });

        MatchManager.itemView = Marionette.ItemView.extend({
            template: '#template2',
            events: {
                'click': 'switch'
            },
            switch: function(){
                MatchManager.main.show(new MatchManager.itemView2(), {preventDestroy: true});
            }
        })

        MatchManager.itemView2 = Marionette.ItemView.extend({
            template: '#template3',
            events: {
                'click': 'back'
            },
            back: function(){
                MatchManager.main.show(MatchManager.layout);
            }
        });

        MatchManager.originalLayout = new MatchManager.layoutView();
        MatchManager.main.show(MatchManager.originalLayout);
    }

    MatchManager.start();

This feels like a major bug. I might be doing something wrong, but I don't think I am. Can anybody here confirm one way or another?

bzalasky commented 10 years ago

Your back method is passing undefined to the show method. You've defined MatchManager.layoutView, but not MatchManager.layout. I'd fix that and see if you're still having any problems.

On Fri, Oct 31, 2014 at 10:07 PM, thisissami notifications@github.com wrote:

Hi All,

I've been getting into Marionette recently and have begun building the front end system for a platform that I've been hired to create. I've ran into what I presume is a bug with Marionette, and wanted to check in here.

In my app, I have a div with id main. This is set up to be a region for my app. In this region, I show a LayoutView. This layout view contains a region, which contains an ItemView. When this view is clicked, the entire main layout is replaced with a new ItemView, with the {preventDestroy: true} attribute passed in. This new ItemView, when clicked, returns the original LayoutView to the main region.

However, the original LayoutView no longer exists when I bring it back to my main region. Perhaps of interest, it is only destroyed after the click in the second ItemView, when the code to bring back to original LayoutView is ran.

Here is the HTML code for my templates:

    <div id="main"></div>
    <script type="text/template" id="template1">
        HELLO HELLO HELLO
        <div id="test"></div>
    </script>
    <script type="text/template" id="template2">
        CLICK HERE TO SWITCH VIEWS
    </script>
    <script type="text/template" id="template3">
        CLICK HERE TO GO BACK
    </script>

And here is my JavaScript code that I run (MatchManager is the name of the Marionette Application, and main is a fully instantiated region).

var MatchManager = new Marionette.Application();
MatchManager.addRegions({
    main: '#main',
});
window.runTest = function(){
    MatchManager.layoutView = Marionette.LayoutView.extend({
        template: '#template1',
        regions: {
            test: '#test'
        },
        onShow: function(){
            this.test.show(new MatchManager.itemView());
        },
    });

    MatchManager.itemView = Marionette.ItemView.extend({
        template: '#template2',
        events: {
            'click': 'switch'
        },
        switch: function(){
            MatchManager.main.show(new MatchManager.itemView2(), {preventDestroy: true});
        }
    })

    MatchManager.itemView2 = Marionette.ItemView.extend({
        template: '#template3',
        events: {
            'click': 'back'
        },
        back: function(){
            MatchManager.main.show(MatchManager.layout);
        }
    });

    MatchManager.originalLayout = new MatchManager.layoutView();
    MatchManager.main.show(MatchManager.originalLayout);
}

MatchManager.start();

This feels like a major bug. I might be doing something wrong, but I don't think I am. Can anybody here confirm one way or another?

— Reply to this email directly or view it on GitHub https://github.com/marionettejs/backbone.marionette/issues/2046.

jasonLaster commented 10 years ago

Thanks @bzalasky! Nice catch

I'm going to close this issue because debug requests belong in stack overflow.

thisissami commented 10 years ago

Hi Guys,

Sorry - that is not the bug. That was a typo that I made while trying to simplify my code for this GitHub post. I had a much more complicated system going on in my code, and didn't want you guys to need to sift through unecessary code.

If you switch MatchManager.layout to MatchManager.layoutView, the code still does not work. I'm fairly certain you guys have a much more serious bug at play here. Here is my output from my console's error log for this particular call stack if this is of any use:

Uncaught TypeError: undefined is not a function backbone.marionette.js:1177_.extend.show backbone.marionette.js:1177MatchManager.itemView2.Marionette.ItemView.extend.back app.js:51jQuery.event.dispatch jquery.js:4641elemData.handle jquery.js:4309

thisissami commented 10 years ago

Ok I feel stupid now. Clearly it should have been changed to MatchManager.originalLayout, and that worked without an issue. Thanks for your help guys... time to figure out what typo I have in my original code!!

samccone commented 10 years ago

:+1:

thisissami commented 10 years ago

So after some further testing, it turns out I didn't even post about the right issue. While the LayoutView remains intact with {preventDestroy:true}, the views that were put into the various regions of the LayoutView get destroyed.

If we switch the definition of MatchManager.layoutView from:

        MatchManager.layoutView = Marionette.LayoutView.extend({
            template: '#template1',
            regions: {
                test: '#test'
            },
            onShow: function(){
                this.test.show(new MatchManager.itemView());
            },
        });

to

        MatchManager.layoutView = Marionette.LayoutView.extend({
            template: '#template1',
            regions: {
                test: '#test'
            },
            onShow: function(){
                if(!this.itemViewed){
                    this.itemViewed = new MatchManager.itemView();
                    this.test.show(this.itemViewed);
                }
            },
        });

The itemView that is put into the test region of the layoutView is destroyed. Of interest, it is only destroyed when the back method is called, not when the switch method is called. Adding a {preventDestroy: true} to the show function in the back method does nothing to change the outcome.

Is LayoutView supposed to work this way, or is there a bug here? If it is supposed to destroy regions within the layout even when preventDestroy is set to true, is there any way to prevent this without changing the Marionette codebase? Is the only alternative to save the model/collection that the view uses and regenerate said view onShow? (this last bit is applicable to my actual code, as opposed to the far simpler examples up above)

jamesplease commented 10 years ago

Quick reply, but I'm 99% sure this will be fixed in 2.3. there's an open PR I'm about to commandeer regarding it

thisissami commented 10 years ago

^thanks for the quick response. What is a "PR" out of curiosity?

Anachron commented 10 years ago

PR = Pull Request. Like a change waiting to be merged into the repository.

thisissami commented 10 years ago

Ah thanks very much for the enlightenment @Anachron

stephanebachelier commented 10 years ago

@jmeas does the PR will fix the issue about showing a view in a layout region in the LayoutView.onShow triggers ? This would be a huge win!

ahumphreys87 commented 9 years ago

@jmeas do you remember which PR this was?

@thisissami have you tried your code with 2.3?

paulfalgout commented 9 years ago

I believe this is the same as https://github.com/marionettejs/backbone.marionette/issues/2415

Essentially the preventDestroy is working, but when you subsequently re-show the layout it will re-render it which rebuilds its regions which will then destroy the child views.

I'm closing this and referencing: https://github.com/marionettejs/backbone.marionette/issues/1263 which contains what would be a fix for this issue.