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

Refreshing with F5 issue #48

Closed
mgscreativa opened this issue Jul 19, 2017 · 14 comments
Closed

Refreshing with F5 issue #48

mgscreativa opened this issue Jul 19, 2017 · 14 comments

Comments

@mgscreativa
Copy link

mgscreativa commented Jul 19, 2017

Hi! If I refresh the page with F5, I don't get any results.

Accessing through defined route, gets the desired page results, but if I press F5 in that route, I don't get any results. I'm currently using Meteor 1.5.1, React 15.5.4 and createContainer HOC.

It seems that on my createContainer props.pagination.ready() never gets true...

Client log by following a route

Pagination users.paginatedList unsubscribe
Pagination devices.paginatedList non-reactive subscribe {} {"fields":{},"sort":{"updatedAt":1},"skip":0,"limit":10,"reactive":false}
Pagination devices.paginatedList find {"sub_c8Jn5fmdRhZ3cCFPu":1} {"fields":{},"sort":{"updatedAt":1}}

Server log by following a route

Pagination devices.paginatedList non-reactive publish {} {"fields":{},"sort":{"updatedAt":1},"skip":0,"limit":10,"reactive":false,"debug":true}

Client log after pressing F5

Pagination devices.paginatedList non-reactive subscribe {} {"fields":{},"sort":{"updatedAt":1},"skip":0,"limit":10,"reactive":false}
Pagination devices.paginatedList unsubscribe

Server log after pressing F5

Pagination devices.paginatedList non-reactive publish {} {"fields":{},"sort":{"updatedAt":1},"skip":0,"limit":10,"reactive":false,"debug":true}

@Kurounin
Copy link
Owner

Kurounin commented Jul 19, 2017

Hi,

Can you post a sample of your code or even better create a github repository with a page/route to replicate the issue?

@mgscreativa
Copy link
Author

Great! will do it asap!

@mgscreativa
Copy link
Author

Hi @Kurounin ! Just pushed my repo to replicate the issue https://github.com/mgscreativa/meteor-kurounin-pagination-refresh-issue

You login as admin with "admin@admin.com" and password as "password" (without the quotes).

After that, go to the "Activities" top menu item and see the list, if you press F5, you will get an endless loading animation!. If you click any item in the "Creator" column you will filter activities by user, and if you press F5, endless loading again...

Another question: I'm implementing the filter by creator (the creator column) by modifying the filters of the pagination object in the createContainer HOC of imports/ui/components/DataGrid/DataGrid.js is that ok or should be implemented elsewhere?

Thanks a lot!

@Kurounin
Copy link
Owner

I spent some time trying to fix this, but the error seems to stem from somewhere else.
When refreshing the Tracker.autorun() which does the subscription when you do this.pagination = new Meteor.Pagination() seems to be invalidated and stopped, which is why the ready is never set to true.
I tried to do a hack and stop Meteor from invalidating that subscription, but it didn't help since changing pages would not work anymore (since the Tracker is still invalidated).
I'll have to investigate it further. Probably the issue lies in how the router is used.
A few things I noticed: in DataGrid.js you should return the actual pagination properties. There's no need to have the additional checks for number of items.

export default createContainer((props) => {
  let filters = {};

  if (props.match.params._id) {
    filters = {
      createdBy: props.match.params._id,
    };
  }

  console.log(filters);

    props.pagination.filters(filters);

    return {
      ready: props.pagination.ready(),
      items: props.pagination.getPage(),
    }
}, DataGrid);

@mgscreativa
Copy link
Author

Thanks @Kurounin ! I moved the filters setup to the Activities page component like this

this.filters = {};
if (match.params._id) {
  this.filters = {
    createdBy: match.params._id,
  };
}

this.pagination = new Meteor.Pagination(ActivitiesCollection, {
  name: 'activities.paginatedList',
  filters: this.filters,
  sort: {},
  perPage: 10,
  reactive: false,
  debug: true,
});

And now my DataCrid createContainer HOC code looks like this

export default createContainer(props => (
{
ready: props.pagination.ready(),
items: props.pagination.getPage(),
}
), DataGrid);

@mgscreativa
Copy link
Author

Regarding the F5 issue will try to remove the datagrid component to see what happens...

@mgscreativa
Copy link
Author

Hi @Kurounin! Please check out my latest push to https://github.com/mgscreativa/meteor-kurounin-pagination-refresh-issue

Test the link Activities No DataGrid It just prints some data using your react example but no luck, if I press F5 it doesn't get ready!

@Kurounin
Copy link
Owner

@mgscreativa Take a look at the PR on your repository.
To fix the issue I moved the loading condition outside the main app component and deferred the rendering of the main app after it loaded to ensure that a separate Tracker computation is used.

@mgscreativa
Copy link
Author

Great! It does work now!

By the way, reviewing my first pagination example here https://github.com/mgscreativa/kurounin-pagination-react-example there's no issue with f5 reloading. The difference between my actual project and the first example repo is that I switched boilerplates, the first one uses a modified version of Meteor Chef Base, and the second uses Meteor Chef PUP, the main difference is that it uses createContainer from react-meteor-data and in the App component it creates a container to track some vars. On the other hand, the first repo uses a custom container HOC in /modules/container created with compose from react-komposer and a custom meteor/tracker in /modules/get-tracker-loader.

I think the issue is that the new boilerplate messes up with the main App component ant that maybe renders Meteor confused about other trackers...

@mgscreativa
Copy link
Author

mgscreativa commented Jul 28, 2017

Well, it almost did it...

Another issue I found here is that if I refresh with F5 and lets say I'm at page 3, the page counter gets reset to page 1...

Clicking on page 3:

Pagination activities.paginatedList non-reactive subscribe {} {"fields":{},"sort":{},"skip":20,"limit":10,"reactive":false}
pagination.js:240 Pagination activities.paginatedList find {"sub_CEdDXxQBiSWjSEuat":1} {"fields":{},"sort":{}}
pagination.js:240 Pagination activities.paginatedList find {"sub_CEdDXxQBiSWjSEuat":1} {"fields":{},"sort":{}}

After refresh with F5:

Pagination activities.paginatedList non-reactive subscribe {} {"fields":{},"sort":{},"skip":0,"limit":10,"reactive":false}
pagination.js:240 Pagination activities.paginatedList find {"sub_LRXBXoZJp7yFdreXh":1} {"fields":{},"sort":{}}
pagination.js:240 Pagination activities.paginatedList find {"sub_LRXBXoZJp7yFdreXh":1} {"fields":{},"sort":{}}

Take a look at the skip it got reset after F5...

@Kurounin
Copy link
Owner

The page gets reset because that information is not kept anywhere (url or local storage). To keep the page after refresh you could watch for changes to the currentPage() of the pagination instance and set the current url to include a page parameter. In the router you will need to modify so it reads the page param from url and feeds it to the pagination (by instantiating the pagination component with the page setting being the url parameter value).

@mgscreativa
Copy link
Author

mgscreativa commented Aug 1, 2017

Thanks @Kurounin! After a revision of the code, It seems harder than I thought to implement your suggestion. In the first place, I can certainly create a new route lets say '/reactive-datatable/page/:page' and I was able to get that param and modify the DataTablesList component through componentWillReceiveProps but I can't set the BootstrapPaginator page to highlight! I saw in the BootstrapPaginator source that will receive a page prop but it seems unimplemented yet.

For the rest, I think it's matter of using componentWillMount and componentWillReceiveProps combined with pagination.currentPage and setState.

Thanks a lot for the advise, if you can review the BootstrapPaginator to accept that page prop will be great.

PS1: At this time, the F5 hack did the trick, now I'm insisting on this for pure hobby on my own
PS2: pagination.currentPage() actually returns a string and not an integer! typeof pagination.currentPage() === string!

@Kurounin
Copy link
Owner

Kurounin commented Aug 2, 2017

BootstrapPaginator doesn't have a page param, because it reads the current page directly from the pagination instance, so once you set the initial page to the pagination there's no need to do anything else for the paginator.
pagination.currentPage() is a number. You probably have it as a string because you didn't typecast it when you set the page from url.

@Kurounin
Copy link
Owner

Kurounin commented Aug 2, 2017

@mgscreativa I created a new PR which implements the page url query param.

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