l-lin / angular-datatables

DataTables with Angular
https://l-lin.github.io/angular-datatables/
MIT License
1.58k stars 487 forks source link

Add the possibility to call DTInstances.getList() and DTInstances.getLast() wherever you are #234

Closed l-lin closed 9 years ago

l-lin commented 9 years ago

At the moment, when calling after the table is rendered, the promise returned by DTInstances.getList() and DTinstances.getLast() are not resolved. One example in this pen.

The issue comes from the fact thoses promises are resolved when the datatables instances (instances generated by DataTables) are registered. Except that the promises are created when calling DTInstances.getList() or DTInstances.getLast(). So the promises are never resolved. In short:

miguelqvd commented 9 years ago

Hello,

First of all thank you for your great job with Angular-DataTables. I'm using it since few weeks and I found issues when I have several tables at the same time under the same controller or different ones when trying to call getList()

I debugged it and I found that getList uses a promise that relies at the same time in another promise _deferLastDTInstances. I've modified the code so we create juste one promise in function register(dtInstance, result) and then forced getList just to return such promise. I'm probably missing something to understand the original implementation but since I have this running everything seems to work great.

Thanks a lot.

Cheers

Miguel-Ángel

function register(dtInstance, result) {
    dtInstance.id = result.id;
    dtInstance.DataTable = result.DataTable;
    dtInstance.dataTable = result.dataTable;

    _instances[dtInstance.id] = dtInstance;

    if (_deferLastDTInstances === null) {
        _deferLastDTInstances = $q.defer();
    }

    _deferLastDTInstances.resolve(dtInstance);

    if (_deferDTInstances === null) {
        _deferDTInstances = $q.defer();
    }

    _deferDTInstances.resolve(_instances);

    return dtInstance;
}

function getLast() {
    return _deferLastDTInstances.promise;
}

function getList() {
    return _deferDTInstances.promise;
}
l-lin commented 9 years ago

Thanks for your suggestion. However, the issue with your solution is that it always use the same promise. As you know, a promise, once resolved, cannot be resolved with another value. So if you change in another page in your SPA that contains another DataTable, the latter will not be registered in the promise. Thus, the DTInstances.getList() and DTInstances.getLast() will not return the latest DataTables instance.

miguelqvd commented 9 years ago

Thank you for your answer. To be honest I'm working with Angular JS since not so long and I'm familiar with promises.

I took your remarks and I did this modifications which also works fine.

How does this look like?

/* @ngInject */ function dtInstances($q) { var _instances = {}; var _lastInstance = {};

return {
    register: register,
    getLast: getLast,
    getList: getList
};

function register(dtInstance, result) {
    dtInstance.id = result.id;
    dtInstance.DataTable = result.DataTable;
    dtInstance.dataTable = result.dataTable;

    _instances[dtInstance.id] = dtInstance;
    _lastInstance = dtInstance;

    return dtInstance;
}

function getLast() {

    var getLastPromise = $q.defer();

    getLastPromise.resolve(_instances);

    return getLastPromise.promise;
}

function getList() {
    var getListPromise = $q.defer();

    getListPromise.resolve(_instances);

    return getListPromise.promise;
}

}

l-lin commented 9 years ago

That won't do either because if you try to get the instances or the last instance before the datatable is registered, then they will both return an empty object. Thanks for your suggestion though.

ajoslin103 commented 9 years ago

I too am having issues with DTInstances.getList()

I call it when a table is clicked on and it fails

TypeError: Cannot read property 'then' of null at Object.getList (angular-datatables.js:457) at controllerFn.vm.setHiliteById (tables-settings.js:35) at controllerFn.vm.rowClicked (tables-settings.js:23) at tables-settings.js:54 at Scope.$get.Scope.$eval (angular.js:14401) at Scope.$get.Scope.$apply (angular.js:14500) at HTMLTableCellElement. (tables-settings.js:53) at HTMLTableCellElement.jQuery.event.dispatch (jquery.js:4430) at HTMLTableCellElement.jQuery.event.add.elemData.handle (jquery.js:4116)

ajoslin103 commented 9 years ago

I spent a few hours mucking about with this and ended up adding and exposing a DTInstances.returnList(){ return _instances; } function -- that gives me a workaround for the broken getList() for now.

I think that getLast and getList could certainly be blocked with a single promise that ensures that at least one instance has been registered and added to the list.

But after that you can no longer block on additional instances because there is no way to know if there will ever be any more tables registered. I suspect that getList() and getLast() can not be implemented after the first instance is registered.

I suggest a getInstance promise requiring a tableId parameter as a way to determine which instance the user is looking for -- and blocking until the instance with that tableId is registered.

And a getList promise that ensures that at least one instance has been registered and added to the list.

getLast is not useful to me - I have 3 tables on a page and using getLast in each of their controllers, but I get the 1st one 3 times because the controllers start up before the DOM is realized and the 1st one to register resolves for all of them.

miguelqvd commented 9 years ago

Hi, I'm probably missing the global picture but I would like to understand why should someone expect something else than an empty object or even an empty array when any datatable hasn't been still registered. Wouldn't be that case something to be handle by the developer?

l-lin commented a day ago

That won't do either because if you try to get the instances or the last instance before the datatable is registered, then they will both return an empty object. Thanks for your suggestion though.

PS: I'm very interested in contributing to this issue as it's already a core component of my solution.

l-lin commented 9 years ago

That means you won't be able to fetch the datatable instances even after the tables are rendered. In short, in sequential order:

miguelqvd commented 9 years ago

Thanks for your clarification. I've created this plunker with those two case of uses:

http://plnkr.co/edit/I1BEU6lIdFkAqxxwGadf?p=preview

When you load the app DTInstances is invoked and a simple list is filled then we can click on a button and fetch DTInstances again.

Sorry if I don't understand your point or if I'm still missing something.

l-lin commented 9 years ago

See your edited plnkr. I just changed displaying the instances from angular to the console. You will notice that the first call in the console does not display the list. Why is it showing when displayed in the $scope? It must be related to angular digest cycle as $q is also using it.

ajoslin103 commented 9 years ago

I submitted a pull request with the code simplified to only promise the first instance, it was the only way I could make getList() work for more than one usage. Let me know if you figure out how to promise additional instances, I sure would like to see how that works !!

poisa commented 9 years ago

I would love to find a solution to this problem too as I can only get the instance right after I create the table but never again. Given that I'm working on a CMS and that I will need to add and remove tables very often, not being able to reload the table data poses a bit of a problem for me!

miguelqvd commented 9 years ago

I'm working on it. By the time being I've managed to have reload functionality running sacrificing the access to DTInstances on page load but the having full access to all the tables when I need it any user interface event:

var refreshItemsTable = function () {
    $scope.selectedItem = null;

    DTInstances.getList().then(function (dtInstances) {
        dtInstances.itemsTable.reloadData();
    });
};

I mean, with the last solution that I proposed and which was incompleted as l-lin showed me in plunkr.

miguelqvd commented 9 years ago

I think I found the solution or at least almost there. Please check my plunkr: http://plnkr.co/edit/I1BEU6lIdFkAqxxwGadf

The issue that I have now us that Angular doesn't apply the results to the user view although the object contains the data expected.

We can see in the console that console.log shows us that the tables were found.

miguelqvd commented 9 years ago

Yes!! :-) It's working!

miguelqvd commented 9 years ago

I've created a pull request.

l-lin commented 9 years ago

Resolved in the PR #242.

poisa commented 9 years ago

This is great! After installing the latest dev branch with this patch I can finally get the instance using getLast(). My problem now is that the table will not reload it's data at all. What am I doing wrong?

    $scope.submit = function()
    {
        $scope.member.put().then(function(member)
        {
            $scope.editFormIsVisible = false;
            $scope.tableIsVisible = true;

            DTInstances.getLast().then(function(dtInstance)
            {
                console.debug('got instance', dtInstance);
                dtInstance.reloadData();
            });
        });
    };

This is the dump I see in Chrome. It does look like there is a function reloadData() but nothing is happening and the table shows the old data until I reload the whole page.

dump

l-lin commented 9 years ago

What renderer are you using? Check this pen. The request is resent to the server when dtInstance.reloadData() is called.

If you are using the Angular renderer, there are no reloadData (check this code). You have to do it manually. It shouldn't be difficult with the two way databinding.

PS: I see you are using a promise inside a promise. You can write better code by chaining promises:

$scope.member.put().then(function(member) {
    $scope.editFormIsVisible = false;
    $scope.tableIsVisible = true;
    return DTInstances.getLast();
}).then(function(dtInstance) {
    console.debug('got instance', dtInstance);
    dtInstance.reloadData();
});
poisa commented 9 years ago

Regarding the chained promises, thanks for the tip. I deeply dislike unnecessarily nested code and having never worked with promises before I didn't know how to solve that!

I don't know exactly what's the name of the renderer I'm using. In fact I couldn't figure out how to get a reference to the data the instance was using so I had to "intercept" it and store it in my scope.

    $scope.tableData = null;

    function serverData(sSource, aoData, fnCallback, oSettings)
    {
        $scope.edit = edit;
        $scope.delete = deleteRow;

        // Inject table name into request 
        aoData.table = 'members';

        oSettings.jqXHR = $.ajax({
            'dataType': 'json',
            'type': 'POST',
            'url': 'http://tsp.dev/datatables',
            'data': aoData,
            'cache': false,
            'success': function(json)
            {
                $scope.$apply(function()
                {
                    // Store a copy of the data in my scope to prevent 
                    // an extra server roundtrip when editing the row
                    $scope.tableData = json;
                });

                fnCallback(json);
            }
        });
    }

    $scope.dtOptions = DTOptionsBuilder.newOptions()
        .withOption('serverSide', true)
        .withPaginationType('full_numbers')
        .withOption('createdRow', createdRow)
        .withFnServerData(serverData);

    $scope.dtColumns = [
        DTColumnBuilder.newColumn('id').withTitle('id'),
        DTColumnBuilder.newColumn('name').withTitle('Name'),
        DTColumnBuilder.newColumn('email').withTitle('Email'),
        DTColumnBuilder.newColumn('birthdate').withTitle('Birthdate'),
        DTColumnBuilder.newColumn(null).withTitle('Actions').notSortable().renderWith(actionsHtml)
    ];

(I had to do all that unfortunate oSettings.jqXHR business because I need to inject an extra post param so that the backend knows which MySQL table to return data from)

gjlloyd commented 9 years ago

i'm using v0.4.2 and am still getting the undefined dtInstances object. the only difference that I see vs the working plunkers is that we are configuring the tables via the dtOptions/dtBuilders instead of just hard coded static tables. we originally upgraded to 0.4 in hopes of resolving the dreaded 'datatables can't reinitialze tables' error, which keeps popping up sporadically in our app. i've read through all these posts but still don't know what to do. we have debugged into the getList() function and can see that my tables are instantiated correctly, but the promise just never gets resolved.

miguelqvd commented 9 years ago

Hi,

Thank you for your feeback. I'm using also DTOptions/DTBuilders on my project and everything is running fine. I've also thought about the solution given and it I don't guess the reason why this is not working for you. Could you please provide a plunkr with an example so we can debug it?