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

Sample blog refactored to MobX and space:tracker-mobx-autorun #1

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

darko-mijic
Copy link
Owner

No description provided.

@RSchwan
Copy link

RSchwan commented Aug 2, 2016

I just had a look at your refactor. It looks really good. Nice job!
I have just one question. Why do you start the the autoruns in theindex.js file? Why don't you start them in the containers when they are needed? You could start them in componentWillMount()and stop them in componentWillUnmount().

@darko-mijic
Copy link
Owner Author

I am rather managing autoruns on the application level.
I don's see the point of managing them in component lifecycle methods. Autoruns can be optimized to unsubscribe based on client side state. For example subscription for fetching post data is not needed while postId is not selected. I will update that.

On the other hand what if you have a situation that multiple components are depending on same autoruns, check must be implemented in order for one component not stop autoruns needed by other components.

Autorun library was created to reduce boilerplate. It was tested on application with hundreds and hundreds of components. The beauty of it is that client side state is managed by autoruns and components are only using data from the state.

@RSchwan
Copy link

RSchwan commented Aug 2, 2016

Ok I see but wouldn't it at least better to call the selectPostand deselectPost actions in the lifecycle methods? This way the router is independent.

@darko-mijic
Copy link
Owner Author

That is very true in this simple example. On the other hand in a real world application there is whole tree of groups of routes, routes and subroutes. Tis way with two simple routing triggers project selection and deselection can be done.

Imagine if you have
/project
/project/details
/project/tasks
/project/teams
/project/schedule
...

@markshust
Copy link

@darko-mijic thank you so much, the autorun placement is what I was also having a problem with -- wondering how to stop autorun on component unmount. by starting all autoruns initially, and only activating them (& stopping them) on state change, i think this is a truly reactive implementation and really dumbs things down for me =)

i'll try to get a similar repo going with mantra implementation.

@RSchwan
Copy link

RSchwan commented Aug 3, 2016

I played a bit with the app and found a performance issue. If you add so 250 comments then the autorun starts to become really slow.
cda1fb2aea941636b7a8a6c73cf426f7
ebe387e07b9716c1655eaafce4d26ca9
It seems like mobx is deep comparing the old array with the new array of trackers autorun. The more items the longer it takes. The rendering on the other hand is fine.

@RSchwan
Copy link

RSchwan commented Aug 3, 2016

Ok I did some more testing. It seems that the mobx logger is causing the bad performance. Without the logger it starts to lag at around 1000 comments. But if you use Mongo.Cursor#observe you can improve performance dramatically. Here is my dirty implementation of the comments autorun:

export default () => {

  // SELECTED POST
  const postId = state.selectedPostId;
  const options = {
    sort: {createdAt: -1}
  };

  let commentsSubscriptionHandle;

  // TODO: manage loading state if subscription is not ready

  if (postId) {
    commentsSubscriptionHandle = Meteor.subscribe('posts.comments', postId);
  } else {
    commentsSubscriptionHandle && commentsSubscriptionHandle.stop();
  }
  //action('updateCommentsFromAutorun', (comments) => {
  //  state.comments.replace(comments);
  //})(postId && commentsSubscriptionHandle.ready() ? Collections.Comments.find({postId}, options).fetch() : []);

  if (postId && commentsSubscriptionHandle.ready()) {
    // fetch comments once and then observe
    Tracker.nonreactive(() => {
      action('init', (comments) => {
        state.comments.replace(comments);
      })(Collections.Comments.find({postId}, options).fetch());
    });

    Collections.Comments.find({postId}, options).observe({
      // we don't want that the addedAt function is triggered x times at the beginning
      // just fetch them once (see above)
      _suppress_initial: true,
      addedAt: action('addedAt', (document, atIndex) => {
        state.comments.splice(atIndex, 0, document);
      }),
      changedAt: action('changedAt', (newDocument, oldDocument, atIndex) => {
        state.comments.splice(atIndex, 1, newDocument);
      }),
      removedAt: action('removedAt', (oldDocument, atIndex) => {
        state.comments.splice(atIndex, 1);
      }),
      movedTo: action('movedTo', (document, fromIndex, toIndex, before) => {
        state.comments.splice(fromIndex, 1);
        state.comments.splice(toIndex, 0, document);
      }),
    });
  } else {
    action('remove', () => {
      state.comments.replace([]);
    })();
  }

};

Maybe it's even possible to abstract this into something like cursorToMobx. Let's see what I can do :)

@darko-mijic
Copy link
Owner Author

Great work @RSchwan, I have prepared autorun update with livequery support based on your solution. It is the first draft. I will push soon so we can discuss and improve on it. I have tried id out here and it simplifies autoruns a lot:

import { Meteor } from 'meteor/meteor';
import { observe } from 'meteor/space:tracker-mobx-autorun';
import * as Collections from '../../lib/collections';
import state from '../store';

export default () => {

  // SELECTED POST
  const postId = state.selectedPostId;
  const options = {
    sort: { createdAt: -1 }
  };

  const commentsSubscriptionHandle = Meteor.subscribe('posts.comments', postId);
  const cursor = Collections.Comments.find({ postId }, options);

  observe('commentsAutorun', state.comments, commentsSubscriptionHandle, cursor);

  return commentsSubscriptionHandle;

};

@darko-mijic
Copy link
Owner Author

To use pre-release autorun package run mgp, mgp needs to be installed first npm install -g mgp.

@RSchwan
Copy link

RSchwan commented Aug 4, 2016

Looks good. I only get a lot of server errors like these:

Exception from sub posts.comments id pk3fZkR9kzMSknHTp Error: Match error: Expected string, got null

@darko-mijic
Copy link
Owner Author

Yes, i saw that. Subscription for comments is expecting postId to be a string check(postId, String);.

@RSchwan
Copy link

RSchwan commented Aug 4, 2016

You just have to do something like this

export default () => {

  // SELECTED POST
  const postId = state.selectedPostId;
  const options = {
    sort: { createdAt: -1 }
  };

  if (postId) {
    const commentsSubscriptionHandle = Meteor.subscribe('posts.comments', postId);
    const cursor = Collections.Comments.find({ postId }, options);
    observe('commentsAutorun', state.comments, commentsSubscriptionHandle, cursor);
  }

};

@darko-mijic
Copy link
Owner Author

Thanks @RSchwan, I will bring that back. We had it before.

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

Successfully merging this pull request may close these issues.

3 participants