Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_LayoutMixin mutates the prototype object panes #695

Open
1 of 5 tasks
green3g opened this issue Mar 8, 2017 · 6 comments
Open
1 of 5 tasks

_LayoutMixin mutates the prototype object panes #695

green3g opened this issue Mar 8, 2017 · 6 comments
Assignees
Labels

Comments

@green3g
Copy link
Member

green3g commented Mar 8, 2017

How often can you reproduce it?

  • Always
  • Sometimes
  • Rarely
  • Unable
  • I didn’t try

Description:

The initPanes function is adding properties to the this.panes object. This results in a object that is quite different from the default if more than one instance of cmv is created.

For instance:

            this.panes.outer = new BorderContainer({
                id: 'borderContainerOuter',
                design: 'sidebar',
                gutters: false
            }).placeAt(container);

This should be avoided because when cmv initializes subsequent times after this object is modified, it fails because lang.clone is unable to handle complex objects like BorderContainer.

Steps to reproduce:

  1. Startup cmv, then try to create a new instance of an app.

Expected results:

CMV should be able to initialize more than once. Why? I am working on an interface that uses cmv in a single page app, and there are situations where cmv is added and removed from the page. This shouldn't be a problem.

Actual results:

CMV fails to initialize a second time.

Environment:

Software Version
CMV Version 2.0.0beta
Browser Chrome
Operating system Windows 7 x64
@green3g green3g self-assigned this Mar 8, 2017
@green3g green3g added the bug label Mar 8, 2017
@tmcgee
Copy link
Member

tmcgee commented Mar 8, 2017

The original panes are preserved in this.defaultPanes so they can be restored before initPanes method is called a second time. Clearly, you'll want to iterate through this.panes to destroy all the existing panes before initializing them again.

@green3g
Copy link
Member Author

green3g commented Mar 8, 2017

Have you messed with destroyRecursive or destroy at all? Should one of those perhaps be implemented?

@green3g
Copy link
Member Author

green3g commented Mar 8, 2017

In theory, if I use dijit/registry and do something like:

registry.toArray().forEach(function(item){
    item.destroyRecursive();
});

It should resolve the issues.

But as you might expect each widget may utilize this at different levels and I can tell already, it is going to be a big problem I think throughout cmv. I think a lot of widgets don't clean up after themselves properly when destroy is called on them, so when they are reinitialized they have issues.

@green3g
Copy link
Member Author

green3g commented Mar 8, 2017

The original panes are preserved in this.defaultPanes so they can be restored before initPanes method is called a second time

Why would initPanes be called a second time unless the app.startup method is called again?

@tmcgee
Copy link
Member

tmcgee commented Mar 8, 2017

Hard to know your intent when you say CMV should be able to initialize more than once. I interpret this to mean starting an app from startup method.

@green3g
Copy link
Member Author

green3g commented Mar 8, 2017

Oh, I misunderstood. So basically what I'm suggesting is this. We have a single page application where cmv is initialized based on the current state of the page. But CMV is not always loaded on the page.

For example:

  1. CMV is started on a dom node with a particular config.
  2. Some other part of the app changes, and cmv is removed (destroyed) from the dom
  3. A new dom node is created and CMV is initialized again, on that new dom node

Basically, on that new dom node, cmv needs to be able to re-initialize itself from scratch, but if there are panes left over from the previous startup these issues will arise.

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

No branches or pull requests

2 participants