martyjs / marty

A Javascript library for state management in React applications
http://martyjs.org
MIT License
1.09k stars 77 forks source link

Setting state on a store that's being listened to causes infinite loop #304

Open anthonator opened 9 years ago

anthonator commented 9 years ago

I have a container that's listening to a store that's fetching remote data via a query. The first time I run through the loop it works fine. However, every subsequent attempt results in an infinite loop. Marty seems to be having issues setting state on a store that it's currently fetching data from.

I provided some code below. Whenever the dispatcher hits an action that sets the loading state of the store it immediately resets the loop. It never gets past dispatching that action.

The specific line is:

this.dispatch(MessageConstants.FETCH_ALL_MESSAGES_STARTING);

from within the query.

My store inherits from a custom base store I created to simplify managing done/load/fail states. I've attached this code as well.

I've been working on this all evening with no luck.

Container

'use strict';

import Marty from 'marty';

// stores
import MessageStore from '../../stores/message-store';

export default function(Component) {
  return Marty.createContainer(Component, {
    listenTo: MessageStore,

    fetch: {
      messages() {
        return MessageStore.for(this).all(this.props.params.id);
      }
    }
  });
}

Query

import Marty from 'marty';

// api
import MessageAPI from '../sources/http_api/message-api';

// constants
import MessageConstants from '../constants/message-constants';

class MessageQueries extends Marty.Queries {
  all(customerId) {
    // infinite loop initiates here
    this.dispatch(MessageConstants.FETCH_ALL_MESSAGES_STARTING);

    return MessageAPI.all(customerId)
      .then(
        (res) => {
          this.dispatch(MessageConstants.FETCH_ALL_MESSAGES, customerId, res.body);
        }
      )
      .catch(
        (err) => {
          this.dispatch(MessageConstants.FETCH_ALL_MESSAGES_FAILED, err);
        }
      );
  }
}

export default Marty.register(MessageQueries, 'MessageQueries');

Store

import Immutable from 'immutable';
import Marty from 'marty';

import BaseStore from './base-store';

// constants
import MessageConstants from '../constants/message-constants';

// queries
import MessageQueries from '../queries/message-queries';

class MessageStore extends BaseStore {
  constructor(options) {
    super(options);

    this.state = this.getInitialState();

    this.state.data = this.state.data.merge({
      collection: undefined
    });

    this.handlers = {
      _addAll: MessageConstants.FETCH_ALL_MESSAGES,

      _done: MessageConstants.FETCH_ALL_MESSAGES,

      _fail: MessageConstants.FETCH_ALL_MESSAGES_FAILED,

      _resetMeta: MessageConstants.FETCH_ALL_MESSAGES_RESET_META,

      _start: MessageConstants.FETCH_ALL_MESSAGES_STARTING
    };
  }

  all(customerId) {
    return this.fetch({
      id: customerId,

      locally: () => {
        let collection = this.state.data.get('collection') || Immutable.List();
        let messages   = collection.get(customerId);

        if (messages) {
          messages = messages.sortBy(
            (customer) => {
              return new Date(customer.get('createdAt'));
            }
          );
        }

        return messages;
      },

      remotely: () => {
        return MessageQueries.all(customerId);
      }
    });
  }

  // private

  _addAll(customerId, messages) {
    let collection = this.state.data.get('collection') || Immutable.Map();
    let values     = collection.get(customerId) || Immutable.List();

    collection = collection.set(customerId, values.merge(messages));

    this.setState({ data: this.state.data.merge({ collection: collection }) });
  }
}

export default Marty.register(MessageStore, 'MessageStore');

Base Store

import Immutable from 'immutable';
import Marty from 'marty';

class BaseStore extends Marty.Store {
  getInitialState() {
    return {
      data: Immutable.Map({
        done: Immutable.Map(),
        errors: Immutable.Map(),
        loading: Immutable.Map(),
        failed: Immutable.Map()
      })
    };
  }

  getBaseActionType() {
    if (this.action.type) {
      return this.action.type.replace(/_DONE|_FAILED|_RESET_META|_STARTING/, '');
    }

    return null;
  }

  didFail(action) {
    return this.state.data.get('failed').get(action.toString()) || false;
  }

  getErrors(action) {
    return this.state.data.get('errors').get(action.toString());
  }

  isDone(action) {
    return this.state.data.get('done').get(action.toString()) || false;
  }

  isLoading(action) {
    return this.state.data.get('loading').get(action.toString()) || false;
  }

  // private

  _done() {
    this._setMeta(true, null, false, false);
  }

  _fail(err) {
    this._setMeta(false, err, true, false);
  }

  _resetMeta() {
    this._setMeta(false, null, false, false);
  }

  _setMeta(done, errors, failed, loading) {
    let d = this.state.data.get('done');
    let e = this.state.data.get('errors');
    let f = this.state.data.get('failed');
    let l = this.state.data.get('loading');

    d = d.set(this.getBaseActionType(), done);
    e = e.set(this.getBaseActionType(), errors);
    f = f.set(this.getBaseActionType(), failed);
    l = l.set(this.getBaseActionType(), loading);

    this.setState(
      {
        data: this.state.data.merge(
          {
            done: d,
            errors: e,
            failed: f,
            loading: l
          }
        )
      }
    );
  }

  _start() {
    this._setMeta(false, null, false, true);
  }
}

export default BaseStore;
anthonator commented 9 years ago

When I navigate away from the component that has the associated container and then come back it works again for the first try and then fails with an infinite loop on subsequent attempts.

anthonator commented 9 years ago

I was able to find a workaround for now. Hopefully this doesn't break something else in the process.

This still seems like a bug to me.

'use strict';

import Marty from 'marty';

// stores
import MessageStore from '../../stores/message-store';

export default function(Component) {
  return Marty.createContainer(Component, {
    listenTo: MessageStore,

    fetch: {
      messages() {
        if (MessageStore.action && MessageStore.action.type === 'FETCH_ALL_MESSAGES_STARTING') { return; }
        return MessageStore.for(this).all(this.props.params.id);
      }
    }
  });
}
taion commented 9 years ago

Do you have a stack trace or anything inside the infinite loop, to show what's triggering the behavior?

Marty is set up not to re-fetch data that's already in the process of being fetched. Is your remotely getting called on subsequent iterations?

anthonator commented 9 years ago

Yes, remotely is being called on subsequent iterations.

Here's the stack trace from Chrome. It's pretty long so I only formatted it until it looked like it started repeating.

Uncaught RangeError: Maximum call stack size exceeded
(anonymous function)                     @ bundle.min.js:1
(anonymous function)                     @ createAssigner.js:37
(anonymous function)                     @ restParam.js:45
(anonymous function)                     @ defaults.js:29
(anonymous function)                     @ restParam.js:44
fetch                                    @ fetch.js:33
all                                      @ conversation-store.js:85
conversations                            @ _conversation-list-container.js:36
(anonymous function)                     @ getFetchResult.js:60
(anonymous function)                     @ createBaseFor.js:19
baseForOwn                               @ baseForOwn.js:14
(anonymous function)                     @ createBaseEach.js:17
(anonymous function)                     @ createForEach.js:16
invokeFetches                            @ getFetchResult.js:56
getFetchResult                           @ getFetchResult.js:12
getState                                 @ createContainer.js:61
onStoreChanged                           @ createContainer.js:48
(anonymous function)                     @ storeObserver.js:52
EventEmitter.emit                        @ events.js:88
_createClass.hasChanged.value.emitChange @ store.js:158
(anonymous function)                     @ actionPayload.js:70
(anonymous function)                     @ createBaseFor.js:19
baseForOwn                               @ baseForOwn.js:14
(anonymous function)                     @ createBaseEach.js:17
(anonymous function)                     @ createForEach.js:16
handled                                  @ actionPayload.js:69
dispatcher.dispatchAction                @ dispatcher.js:38
dispatch                                 @ dispatchCoordinator.js:37
all                                      @ conversation-queries.js:47
remotely                                 @ conversation-store.js:102
tryAndGetRemotely                        @ fetch.js:87
fetch                                    @ fetch.js:66
all                                      @ conversation-store.js:85
conversations                            @ _conversation-list-container.js:36
(anonymous function)                     @ getFetchResult.js:60
(anonymous function)                     @ createBaseFor.js:19
baseForOwn                               @ baseForOwn.js:14
(anonymous function)                     @ createBaseEach.js:17
(anonymous function)                     @ createForEach.js:16
invokeFetches                            @ getFetchResult.js:56
getFetchResult                           @ getFetchResult.js:12
getState                                 @ createContainer.js:61
onStoreChanged                           @ createContainer.js:48
(anonymous function)                     @ storeObserver.js:52
EventEmitter.emit                        @ events.js:88
_createClass.hasChanged.value.emitChange @ store.js:158
(anonymous function)                     @ actionPayload.js:70
(anonymous function)                     @ createBaseFor.js:19
baseForOwn                               @ baseForOwn.js:14
(anonymous function)                     @ createBaseEach.js:17
(anonymous function)                     @ createForEach.js:16
handled                                  @ actionPayload.js:69
dispatcher.dispatchAction                @ dispatcher.js:38
dispatch                                 @ dispatchCoordinator.js:37
all                                      @ conversation-queries.js:47
remotely                                 @ conversation-store.js:102
tryAndGetRemotely                        @ fetch.js:87
fetch                                    @ fetch.js:66
all                                      @ conversation-store.js:85
conversations                            @ _conversation-list-container.js:36
(anonymous function)                     @ getFetchResult.js:60
(anonymous function)                     @ createBaseFor.js:19
baseForOwn                               @ baseForOwn.js:14
(anonymous function)                     @ createBaseEach.js:17
(anonymous function)                     @ createForEach.js:16
invokeFetches                            @ getFetchResult.js:56
getFetchResult                           @ getFetchResult.js:12
getState                                 @ createContainer.js:61
onStoreChanged                           @ createContainer.js:48
(anonymous function)                     @ storeObserver.js:52
EventEmitter.emit                        @ events.js:88
_createClass.hasChanged.value.emitChange @ store.js:158
(anonymous function)                     @ actionPayload.js:70(anonymous function) @ createBaseFor.js:19baseForOwn @ baseForOwn.js:14(anonymous function) @ createBaseEach.js:17(anonymous function) @ createForEach.js:16handled @ actionPayload.js:69dispatcher.dispatchAction @ dispatcher.js:38dispatch @ dispatchCoordinator.js:37all @ conversation-queries.js:47remotely @ conversation-store.js:102tryAndGetRemotely @ fetch.js:87fetch @ fetch.js:66all @ conversation-store.js:85conversations @ _conversation-list-container.js:36(anonymous function) @ getFetchResult.js:60(anonymous function) @ createBaseFor.js:19baseForOwn @ baseForOwn.js:14(anonymous function) @ createBaseEach.js:17(anonymous function) @ createForEach.js:16invokeFetches @ getFetchResult.js:56getFetchResult @ getFetchResult.js:12getState @ createContainer.js:61onStoreChanged @ createContainer.js:48(anonymous function) @ storeObserver.js:52EventEmitter.emit @ events.js:88_createClass.hasChanged.value.emitChange @ store.js:158(anonymous function) @ actionPayload.js:70(anonymous function) @ createBaseFor.js:19baseForOwn @ baseForOwn.js:14(anonymous function) @ createBaseEach.js:17(anonymous function) @ createForEach.js:16handled @ actionPayload.js:69dispatcher.dispatchAction @ dispatcher.js:38dispatch @ dispatchCoordinator.js:37all @ conversation-queries.js:47remotely @ conversation-store.js:102tryAndGetRemotely @ fetch.js:87fetch @ fetch.js:66all @ conversation-store.js:85conversations @ _conversation-list-container.js:36(anonymous function) @ getFetchResult.js:60(anonymous function) @ createBaseFor.js:19baseForOwn @ baseForOwn.js:14(anonymous function) @ createBaseEach.js:17(anonymous function) @ createForEach.js:16invokeFetches @ getFetchResult.js:56getFetchResult @ getFetchResult.js:12getState @ createContainer.js:61onStoreChanged @ createContainer.js:48(anonymous function) @ storeObserver.js:52EventEmitter.emit @ events.js:88_createClass.hasChanged.value.emitChange @ store.js:158(anonymous function) @ actionPayload.js:70(anonymous function) @ createBaseFor.js:19baseForOwn @ baseForOwn.js:14(anonymous function) @ createBaseEach.js:17(anonymous function) @ createForEach.js:16handled @ actionPayload.js:69dispatcher.dispatchAction @ dispatcher.js:38dispatch @ dispatchCoordinator.js:37all @ conversation-queries.js:47remotely @ conversation-store.js:102tryAndGetRemotely @ fetch.js:87fetch @ fetch.js:66all @ conversation-store.js:85conversations @ _conversation-list-container.js:36(anonymous function) @ getFetchResult.js:60(anonymous function) @ createBaseFor.js:19baseForOwn @ baseForOwn.js:14(anonymous function) @ createBaseEach.js:17(anonymous function) @ createForEach.js:16invokeFetches @ getFetchResult.js:56getFetchResult @ getFetchResult.js:12getState @ createContainer.js:61onStoreChanged @ createContainer.js:48(anonymous function) @ storeObserver.js:52EventEmitter.emit @ events.js:88_createClass.hasChanged.value.emitChange @ store.js:158(anonymous function) @ actionPayload.js:70(anonymous function) @ createBaseFor.js:19baseForOwn @ baseForOwn.js:14(anonymous function) @ createBaseEach.js:17(anonymous function) @ createForEach.js:16handled @ actionPayload.js:69dispatcher.dispatchAction @ dispatcher.js:38dispatch @ dispatchCoordinator.js:37all @ conversation-queries.js:47remotely @ conversation-store.js:102tryAndGetRemotely @ fetch.js:87fetch @ fetch.js:66all @ conversation-store.js:85conversations @ _conversation-list-container.js:36(anonymous function) @ getFetchResult.js:60(anonymous function) @ createBaseFor.js:19baseForOwn @ baseForOwn.js:14(anonymous function) @ createBaseEach.js:17(anonymous function) @ createForEach.js:16invokeFetches @ getFetchResult.js:56getFetchResult @ getFetchResult.js:12getState @ createContainer.js:61onStoreChanged @ createContainer.js:48(anonymous function) @ storeObserver.js:52EventEmitter.emit @ events.js:88_createClass.hasChanged.value.emitChange @ store.js:158(anonymous function) @ actionPayload.js:70(anonymous function) @ createBaseFor.js:19baseForOwn @ baseForOwn.js:14(anonymous function) @ createBaseEach.js:17(anonymous function) @ createForEach.js:16handled @ actionPayload.js:69dispatcher.dispatchAction @ dispatcher.js:38dispatch @ dispatchCoordinator.js:37all @ conversation-queries.js:47remotely @ conversation-store.js:102tryAndGetRemotely @ fetch.js:87fetch @ fetch.js:66all @ conversation-store.js:85conversations @ _conversation-list-container.js:36(anonymous function) @ getFetchResult.js:60(anonymous function) @ createBaseFor.js:19baseForOwn @ baseForOwn.js:14(anonymous function) @ createBaseEach.js:17(anonymous function) @ createForEach.js:16invokeFetches @ getFetchResult.js:56getFetchResult @ getFetchResult.js:12getState @ createContainer.js:61onStoreChanged @ createContainer.js:48(anonymous function) @ storeObserver.js:52
anthonator commented 9 years ago

This is still causing my team a lot of headache. It doesn't seem like anybody else is seeing this issue so it must be us.

Is it valid in Marty to use a store's fetch method within a container?

e.g.

Customer Store

get(id) {
  return this.fetch({
    id: id,

    locally() {
      let collection = this.state.data.get('collection') || Immutable.Map();

      return collection.get(id);
    },

    remotely() {
      return CustomerQueries.get(id);
    }
  });
}

Container

fetch: {
  customer() {
    let customerId = this.context.router.getCurrentParams().id;

    return CustomerStore.for(this).get(customerId);
  }
}
taion commented 9 years ago

Can you confirm which version of Marty you're using?

anthonator commented 9 years ago

0.9.17

taion commented 9 years ago

Shoot. Okay. That's legitimately a bug. The issue is that as long as we dispatch actions synchronously, this line: https://github.com/martyjs/marty-lib/blob/v0.10.6/src/store/storeFetch.js#L83 needs to be hoisted before the remotely call, in case the remotely call synchronously triggers a dispatch.

The problem is that right now we're already on v0.10.x, which has breaking changes if you're on v0.9.x (although I think the API for isomorphism is much much nicer).

taion commented 9 years ago

I guess maybe most other people aren't bothering to explicitly handle the _STARTING action when using fetch.

If you want to work around this right now, you should be able to just wrap either

this.dispatch(MessageConstants.FETCH_ALL_MESSAGES_STARTING);

or

_start() {
  this._setMeta(false, null, false, true);
}

in a setTimeout with 0 timeout or whatever other way you want to defer it.

anthonator commented 9 years ago

Ok, I have a workaround but there have been some circumstances where it doesn't work. Maybe setTimeout will do a better job.

Any chance this can be fixed in 0.9 or will I have to switch to 0.10?

taion commented 9 years ago

@jhollingworth Do you think it's worth cutting a v0.9.18 to address this?

anthonator commented 8 years ago

Any updates here? Has this been fixed in 0.10?

taion commented 8 years ago

Not yet. I can patch it in one or either, but need @jhollingworth to cut an NPM release.

anthonator commented 8 years ago

Any updates on this?

anthonator commented 8 years ago

Just saw the README. Super sorry to see this project go. Thanks for all your hardwork!