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 pagination on atmosphere #42

Open
caseycobb opened this issue May 30, 2013 · 9 comments
Open

Add pagination on atmosphere #42

caseycobb opened this issue May 30, 2013 · 9 comments

Comments

@caseycobb
Copy link

We need a pager on the home page. Tom - you actually suggested this but I don't see a ticket for it, so I'm creating one!

@caseycobb
Copy link
Author

@tmeasday - we'll handle this one if you're cool with that.

@tmeasday
Copy link
Member

Awesome!

@spyrosk
Copy link

spyrosk commented Jul 4, 2013

Hi @tmeasday,

We've been going over solutions for this and devised a couple different options. Before proceeding, we'd like some thoughts from the community.

  1. Handle the collections using standard publish/subscribe functions, passing the pageNum and search parameters as arguments stored in client session variables. The one problem we foresee is the server will need to track all the subscriptions made by the various clients. I don't see the page number as a huge issue, but the possibilities of search parameters could very widely and may require additional resources on the server side to track the server side collections for changes. We may be over thinking this, but want to be conscious of the impact server side.
  2. Perform the pagination on the client side completely. For now this should work nicely, but as the collection grows it could cause issues down the line.
  3. Make a more traditional server/client modal, where the client simply makes a server call and asks for the results it was interested in, avoiding the publishing and requiring for the server to track the changes in the collection. This may not be much better than option 1, as the server ends up making many of the same calls over and over.

This is really more a question on performance and making sure we build something that can scale. Do you have any specific preference, or thoughts on the proposed solutions?

@tmeasday
Copy link
Member

Hi @spyrosk,

I think it has to be 1., although I can see the argument why, with essentially static data, it feels a bit painful.

Here are some of my thoughts:

a) A large proportion of clients will use the same query (page 1, no search).

b) The majority of users aren't going to stick around for very long.

c) There'll be a potential memory cost of having a lot of search subscriptions open, but my guess is it'll be OK, given b).

d) I wouldn't worry about CPU costs as the data doesn't change very much.

e) Doing it client side is rapidly becoming infeasible - even with only 414 packages, it already takes me about 5-10 seconds to load the data over the wire. (Although it's possible we are sending more data than is needed to the client in the packages collection and this number could be reduced a fair bit - we should fix this either way).

I don't know if you have already, but you should take a look at the paginated subscription package that we use for Telescope (and in the book). Also I believe there are a couple of other pagination packages on atmosphere that are probably worth a look.

@stephengpope
Copy link

Hi @tmeasday

Thanks for the feedback, we'll get right on this.

@spyrosk
Copy link

spyrosk commented Jul 24, 2013

Hi @tmeasday ,

I've implemented the separate subscriptions to implement the pagination, however I'm having some trouble getting the search keywords to work.
Specifically, if I type a single letter, or paste the whole word that I want to search for, then the packages subscription (and results) are filtered correctly. However if I type the word in the search box, one letter at a time, then after the first letter the results no longer update and remain this way until I delete the whole word. For some reason the subscription isn't updated when I type multiple characters separately.

Could you please have a quick look at my code and see if you have any guidance for me?

As this isn't ready for merging I didn't create a pull request, but please let me know if you'd like me to. Here is a link to the branch where I've implemented my changes: https://github.com/spyrosk/atmosphere/tree/issue-42

Thank you.

@tmeasday
Copy link
Member

Hey @spyrosk,

Looks like a good start. Here are some comments:

  1. Can't see anything obvious that explains that weird behaviour. Would need to debug into it to figure out why it's happening.
  2. It's a pity the packageCount pub has to be so complex (I know it has to be). Perhaps it could be simplified by reusing the observeChanges callbacks.
  3. You could put the packageCount publication code inside the packages publication. That would save calculating the same query and remembering to pass the same parameters to both. Or you could just generate the finder in a helper function.

@yeputons
Copy link
Contributor

@tmeasday @spyrosk Where did you land on this one? I'm having difficulties viewing the main page of Atmosphere right now - CPU is loaded for 25% (full load on one core) once I go to atmosphere.meteor.com and Firefox slows down dramatically. I think that very-very basic pagination (like, pass additional 'limit' parameter to publication and add infinite scroll if there is no search) will be more than enough to get rid of these performance issues.

@Tarang
Copy link
Member

Tarang commented Feb 22, 2014

@yeputons I believe this issue is solved in a different way by the new atmosphere version. See beta.atmospherejs.com. The package list layout is no longer the design used for atmosphere.

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

6 participants