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

DTInstance.getList() possible memory leak #284

Closed
heliodor opened this issue Apr 21, 2015 · 11 comments
Closed

DTInstance.getList() possible memory leak #284

heliodor opened this issue Apr 21, 2015 · 11 comments
Milestone

Comments

@heliodor
Copy link

The docs say:

"A DataTable directive instance is created each time a DataTable is rendered. You can fetch it by calling ... DTInstances.getList() to fetch the entire list of instances."

I did notice the same, where as I render the table over and over, the list returned by getLast() grows bigger and bigger.

Isn't this, then, a memory leak?

If this behavior is going to stay, how can I make sure previous instances get removed?

@mathewbergt
Copy link
Contributor

Related to issue #282.

@l-lin
Copy link
Owner

l-lin commented Apr 23, 2015

Yup, I did not think about how to clear the list yet. Still thinking...

@heliodor
Copy link
Author

I'm curious, what is the purpose of having the historical list of directive objects? I would think having only the current items would be enough.

@l-lin
Copy link
Owner

l-lin commented Apr 23, 2015

It's because I'm registring the instances one by one. It's not possible to know in advance how many datatables are instanciated.
That's why there is a "historical" list of instances, altough it was only intented to list the current instances.

Maybe a solution is to check, each time a datatables is registred, if in the list each instance is still in the DOM or not.

@l-lin l-lin added this to the v0.4.3 milestone Apr 24, 2015
@l-lin
Copy link
Owner

l-lin commented Apr 24, 2015

I added a timeout (1s) after which the instances that are not present in the page are removed.
If anyone has a better idea...

@l-lin l-lin closed this as completed Apr 24, 2015
@heliodor
Copy link
Author

Has anyone ever said they need the whole list of instances currently on the page? I don't think having this list is necessary. A user can keep track of their needs based on the options or data, rather than the actual DataTables objects.

I'd imagine the majority of use cases for DTInstances.getList() are to get the corresponding DataTables instance, in which case there has to be a better way to give the user that particular instance instead of a list of all instances from which to pick it.

This is really a shortcoming in the DataTables implementation. They should be passing the instance to all callbacks. In my project's case for example, I use a rowCallback to expand and collapse a child row and I need the DataTables instance in order to do so. DT doesn't give it to me, so I have to get it from you using DTInstances.getList()[someIndex]. I shouldn't have to do this. The DT instance should be given to me as a param of the callback.

I think if DataTables would pass the instance to all their callbacks, your DTInstances.getList() and DTInstances.getLast() would not be needed anymore. As an interim solution, you could take one of these approaches:

  1. Remove the DTInstances module and provide a wrapper for each and every callback available in DataTables. For example, instead of specifying "rowCallback: function() {}", I'd specify "angRowCallback: function() {}" and you'd pick that up in your directive, create the DataTables instance, and create a closure with it around my callback function.

  2. The user could specify a location in their $scope where your directive can place the DataTables instance once it's created. Again, DTInstances would not be needed anymore. Then, the user would have a one-to-one link between DataTables instances and the data sources or options objects that were used to create the tables.

@heliodor
Copy link
Author

Searching through the code, I don't see any calls to getLast() or getList(), so you don't need the list of instances for internal use, right?

@l-lin
Copy link
Owner

l-lin commented Apr 24, 2015

It's not used internally. Its purpose is only for post-render operations like your example or reloading/rerender the datatables.

Interesting ideas 👍 I'll see what I can change.

@heliodor
Copy link
Author

Both my earlier ideas are rather roundabout. Here's a more direct approach:

Just like you have the dtOptions and dtColumns attributes in the element tag, you should let the user specify a dtInstanceCallback attribute through which the user will specify the name of a callback. You'd call that function and pass it the DT instance every time it changes. The user can easily take it from there. Your DTInstances would not be needed anymore at that point.

@l-lin
Copy link
Owner

l-lin commented Apr 24, 2015

Why didn't I think about this simple solution 😅
Thanks @heliodor! You're awesome!

@techvn
Copy link

techvn commented Jan 11, 2017

How can i get current DTInstance if not set in html template?

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

No branches or pull requests

4 participants