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

pager action checks if data source is a function #53

Closed
wants to merge 1 commit into from

Conversation

rwilcox
Copy link

@rwilcox rwilcox commented Sep 22, 2016

Howdy!

There's a bug report in this pull request, and a bit of a story, so grab a chair...

Background: I'm using react-redux-grid with a dataSource which is a custom function that returns a promise. (This lets me add authorization and header details I need for the remote API).

I also want my paging type to be 'remote' also.

I believe(d) that this page configuration would work:

            PAGER: {
                enabled: true,
                pagingType: 'remote'
                        }

and that react-redux-grid would use the dataSource I had set up.

In fact, what happens is PagerActions setPageAsync function's dataSource parameter set to { currentRecords: ..., data: ..., gridType: ...., lastUpdate: ...., proxy: ...., total: ...., treeData: ...}. What parent components call gridData.

Regardless why this gets passed an object, when it does pass an object, it tries to make an AJAX request to http://[Object]. (See https://github.com/bencripps/react-redux-grid/blob/master/src/actions/plugins/pager/PagerActions.js#L105 ). It's trying to serialize the gridData structure as a string then call that host.

But even in this object there's no reference back to my promise (that I could see).

Sooooo I set PAGER.pagingSource explicitly to the same function as I use for dataSource.

Which meant that setPageAsync's dataSource was what I wanted - a function! Except I immediately ran into the same bug as above: trying to send an AJAX request to http://[Function].

SOOOOOOO this patch: check to see if dataSource is a function. If it is, use that to get a promise. Else, call Request.api as before.

There is still a bug here: if you set pagingType to remote, but don't provide a pagingSource, why is the data source a record? Don't know - in my use case I'm fine with having pagingSource need to be provided / a function (in fact, my code will have to be structured that way for other reasons... like setPageAsync not knowing the current sorting of the grid from down in the PagerActions Plain Old Javascript Class. (If there's a way let me know ;) )

Let me know if there's anything I can do to get this accepted. Thanks!

@bencripps
Copy link
Owner

Good catch on this bug! Most implementation are using a custom pager, so this hasn't been uncovered yet!

Let me take some time to think about this issue, and review your changes. I think there might be a larger fix necessary, but I'd like to ponder it for a bit. I'll post some more comments tomorrow once I've been able to review the changes.

Thanks!

@bencripps bencripps added the bug label Sep 22, 2016
bencripps added a commit that referenced this pull request Sep 30, 2016
bencripps added a commit that referenced this pull request Sep 30, 2016
bencripps added a commit that referenced this pull request Sep 30, 2016
bencripps added a commit that referenced this pull request Sep 30, 2016
@bencripps
Copy link
Owner

@rwilcox The newest version of grid fixes this issue with the pager component, removing the pagingSource prop, relying on the dataSource function if paging type is remote.

Let me know if this fixes your issue.

@bencripps bencripps closed this Oct 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants