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

Side effects like changing state are not allowed at this point #18

Closed
0x80 opened this issue Jun 6, 2018 · 8 comments
Closed

Side effects like changing state are not allowed at this point #18

0x80 opened this issue Jun 6, 2018 · 8 comments

Comments

@0x80
Copy link

0x80 commented Jun 6, 2018

I am quite new to Mobx so I suspect this problem might not be be Firestorter specific, but I'm hoping you have clue anyway.

I have a list of document ids what I use to fetch single document data one by one. I have a store which contains a Document, of which I change the path property via a "Next" button click handler which calls an @action.bound handler on the store and sets the next id.

I initially create the document without any path, with new Document().

When the next button triggers the action and the document path is set, I wait for document.fetching to be false and pass it down to a component for rendering.

When I hit the button and the path is set, I get this from my rendering component:

Side effects like changing state are not allowed at this point. Are you trying to modify state from, for example, the render function of a React component? Tried to modify: ObservableValue@16

The problem seems to stem from the fact that I'm passing down the observable Document as-is.

 <RegistrationShortView document={store.userRegistration} />

If I instead do this (map it to my own non-observable typed document structure), all is ok:

 <RegistrationShortView
    document={{
      id: store.userRegistration.id,
      data: store.userRegistration.data as UserRegistration
    }}
 />

The component does nothing more then de-structure some of the data props and renders them.

What might be causing this issue you think?

@IjzerenHein
Copy link
Owner

Hi Thijs, I've only recently encountered this myself. The problem lies in the fetching prop which doesn't trigger a start of real-time updating in the higher up component, but it does in a component further down in the tree. The solution is to not only access the fetching prop but also the data prop in the component higher up.
Let me know whether that works.

@0x80
Copy link
Author

0x80 commented Jun 6, 2018

🙌 🎉

I was getting pretty desperate trying out all different things for about a day 😅

I solved it by forcing mode = "on" on all documents:

  public user = new FsDocument<User>(undefined, { mode: "on" });
  public userRegistration = new FsDocument<UserRegistration>(undefined, {
    mode: "on"
  });
  public userCredits = new FsDocument<UserCredits>(undefined, { mode: "on" });
  public userMetrics = new FsDocument<UserMetrics>(undefined, { mode: "on" });

PS: I called your Document type FsDocument because I already had a Document of my own.

@IjzerenHein
Copy link
Owner

IjzerenHein commented Jun 6, 2018

Thijs, could you show how you are using the fetching prop in your setup? I'm suspecting that to be related to the problem. The reason being that fetching is a bit special, it is the one of the few observables that may change during the execution of the render function (which may lead to such problems and I might change this behavior to be debounced).

@0x80
Copy link
Author

0x80 commented Jun 11, 2018

I am passing the observable documents into my view components and show a loading indicator. Something like below. Document<T> is just a typed version of a regular Firestorter document.

type QuickAcceptViewProps = {
  user: Document<User>;
  userRegistration: Document<UserRegistration>;
};

@observer
export default class QuickAcceptView extends React.Component<
  QuickAcceptViewProps
> {
  public render() {
    const { user, userRegistration } = this.props;
    if (user.fetching || userRegistration.fetching) {
      return <LoadingIndicator />;
    }

    const { data: userData, id: userId } = user;
    const { data: userRegistrationData } = userRegistration;

    // ... render actual data 

@IjzerenHein
Copy link
Owner

Ah I see.

I think I know what the problem is. Accessing just the fetching property does not cause the documents to start real-time updating. In order for that to happen, the actual data needs to be accessed.

I ran into this myself and was thinking of a making a change to firestorter to mitigate this. I was thinking of introducing a property called isLoading which will replace fetching. Unlike fetching, isLoading will also start real-time updating and would make that code work. I've already used it in the project I'm working on and it works well.

As temporary solution you could try adding the isLoading property yourself to your classes. The implementation would be very simple:

get isLoading() {
  const { data, fetching } = this;
  return fetching;
}

Let me know whether that helps.

@0x80
Copy link
Author

0x80 commented Jun 11, 2018

@IjzerenHein Thanks. For now my solution works with setting mode = on, but I'm looking forward to using the new isLoading getter. 👍for using the is* prefix 😄

@IjzerenHein
Copy link
Owner

Hi, this should be fixed by using the new isLoading property instead of fetching in firestorter v0.12.1. Let me know if you still run into this problem with in case isLoading doesn't solve it.
cheers

@steeltomato
Copy link

I can't reproduce this anymore on v0.12.1

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

3 participants