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

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

Open
anthonator opened this issue May 6, 2015 · 15 comments
Open

Comments

@anthonator
Copy link

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

taion commented Jun 15, 2015

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

taion commented Jun 30, 2015

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

@anthonator
Copy link
Author

0.9.17

@taion
Copy link
Member

taion commented Jun 30, 2015

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
Copy link
Member

taion commented Jun 30, 2015

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
Copy link
Author

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
Copy link
Member

taion commented Jun 30, 2015

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

@anthonator
Copy link
Author

Any updates here? Has this been fixed in 0.10?

@taion
Copy link
Member

taion commented Jul 23, 2015

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

@anthonator
Copy link
Author

Any updates on this?

@anthonator
Copy link
Author

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

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