martyjs / marty

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

Store.hasChanged being called excessively #289

Closed oliverwoodings closed 9 years ago

oliverwoodings commented 9 years ago

Probably a question for @jhollingworth to answer, although would appreciate any other input from the rest of @martyjs/core. I believe fetch is making excessive calls to Store.hasChanged, especially when it comes to handling fetch dependencies. It seems that after a fetch resolves, no matter what happens, a store update is triggered which causes all dependent containers to re-fetch and all subsequent components to re-render.

Maybe my understanding of how fetch is working internally is flawed, but to me it doesn't make sense that a fetch directly triggers a store change. Surely a change should only be triggered if the store has actually been mutated?

Furthermore I don't understand why a fetch using dependsOn triggers yet another fetch - surely this one is completely pointless, because the call above to store.fetch(options) will result in one being done anyway?

This behaviour was brought to my attention by large parts of my app being re-rendered 8 times on load when only 3 queries (and ultimately 3 calls to store.replaceState) were called. I know React and Flux are designed to support regular re-rendering, but this just seems a little wasteful?

taion commented 9 years ago

I've noticed the same thing. It's a little tricky, though. What's happening is:

  1. Fetch triggered by container.
  2. Local fetch fails.
  3. Remote fetch triggered, fetch marked as "in progress".
  4. Remote fetch succeeds and dispatches action.
  5. Store picks up dispatched action, marks itself as having changed.
  6. Fetch triggered by container, sees that fetch is in progress and returns pending.
  7. Success continuation on remote fetch invoked, fetch marked as "finished"
  8. Something has to make the container re-render with the now-present fetch results.

Within this setup, unless you can somehow get the fetch to be marked as no longer being "in progress" before the dispatched action gets picked up by the store, it's not clear how you'd get around the extra store update.

oliverwoodings commented 9 years ago

Ah I think I understand it better now - can see why fetches need to trigger changes. Guess there isn't any way to solve this without completely refactoring how fetches work internally

jhollingworth commented 9 years ago

I'm going to close this for now. Please re-open if you've got more thoughts on the matter