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

Add developer warning when replacing SC.ArrayController content #1364

Open
mauritslamers opened this issue May 5, 2016 · 2 comments
Open

Comments

@mauritslamers
Copy link
Member

mauritslamers commented May 5, 2016

A well known source for memory leaks in SproutCore applications is replacing a SC.RecordArray instance with a new one as content in an array controller without destroying the original instance first. This causes a memory leak because the store will hold on to a reference of the record array instance in order to update it whenever new information is available.

I propose to add the following to SC.ArrayController in order to give a developer warning when someone tries to replace a record array instance which is not destroyed. As SC.ArrayController is in the core_foundation framework, it uses a simple check on properties that need to exists on a record aray instead of using SC.instanceOf(SC.RecordArray).

  set: function (key, value) {
    if (key === "content") {
      var c = this.get('content');
      if (c.store || c.query) {
        if (!c.get('isDestroyed')) {
          SC.warn("Developer Warning: You are replacing the content of an array controller with a new record array without destroying the old one. This causes memory leaks!");
        }
      }
    }
    sc_super();
  },

Edit: grammar

@nicolasbadia
Copy link
Member

I really like the idea, I will give it a try.
I suggest that we wrap this in //@if (debug) statements.

@mauritslamers
Copy link
Member Author

It should most certainly go in an //@if debug block. On top of SC.ArrayController there is already one with toString. I intended it to go below (that is where I wrote it). I am just not sure whether this should go to master now, or to 2-0-unstable.

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

No branches or pull requests

2 participants