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

Subtle caveat with async methods #184

Open
k1w1 opened this issue May 20, 2020 · 4 comments
Open

Subtle caveat with async methods #184

k1w1 opened this issue May 20, 2020 · 4 comments
Labels

Comments

@k1w1
Copy link

k1w1 commented May 20, 2020

React Easy State version: 6.3.0
Platform: browser

There is a subtle caveat when state is modified after an async function, or in any situation where the code is called asynchronously outside a React render cycle.

If a reference to new state is held across an async boundary then the new state is not observed and changes to it do no trigger a re-render.

Here is a code snippet that shows the problem. The key is that a local variable counter is used after the async call. If it is used before the async call it works correctly, or if counterStore is used after the call to get a reference to the value from the store again then it also works correctly.

import React from 'react';
import { store, view } from '@risingstack/react-easy-state';

const counterStore = store({
  counters: []
});

async function longFunction() {}

export default view(() => {
  const addCounter = async () => {
    counterStore.counters.push({value: 0});
    const counter = counterStore.counters[counterStore.counters.length-1];

    await longFunction();

    counter.value++;
    console.log("No re-render here!")
  }
  const incCounter = () => {
    counterStore.counters[counterStore.counters.length-1].value++;
  }
  return (
    <>
      <button onClick={() => addCounter()}>Add counter</button>
      <button onClick={() => incCounter()}>Increment counter</button>
      <ul>
        {counterStore.counters.map((counter, index) => (
          <li key={index}>{counter.value}</li>
        ))}
      </ul>
    </>
  );
});

It seems like you could just say "always reference the store when updating a value", but that is very hard to do, especially with complex code. The same above was created by removing all of the complexity and nested function calls that was otherwise hiding the problem.

This is disappointing because it undermines the awesome promise of react-easy-state. I think it is worth trying to solve so code like this can work transparently.

Theory of the problem

I think the problem exists because of this sequence:

  1. addCounter is called from an event handler, so changes to the state are proxied (observed), but the new values themselves are not wrapped in a proxy.
  2. This means that counter is not a proxied variable, even though it appears to have been correctly read from the store.
  3. Without the await function it works fine because the re-render is triggered when the element is added to the counterStore.counters array and the render doesn't happen until after the function exits (because React is batching the renders).

Second variation on problem

The same problem can be triggered using setTimeout too:

import React from "react";
import { store, view } from "@risingstack/react-easy-state";

const counterStore = store({
  counters: []
});

export default view(() => {
  const addCounter = () => {
    setTimeout(() => {
      counterStore.counters.push({ value: 0 });
      const counter = counterStore.counters[counterStore.counters.length - 1];
      setTimeout(() => {
        counter.value++;
        console.log("No re-render here!");
      }, 1000);
    }, 1000);
  };
  const incCounter = () => {
    counterStore.counters[counterStore.counters.length - 1].value++;
  };
  return (
    <>
      <button onClick={() => addCounter()}>Add counter</button>
      <button onClick={() => incCounter()}>Increment counter</button>
      <ul>
        {counterStore.counters.map(counter => (
          <li>{counter.value}</li>
        ))}
      </ul>
    </>
  );
});
@k1w1 k1w1 added the bug label May 20, 2020
@solkimicreb
Copy link
Member

This seems like a very valid issue, luckily the fix will probably be easy and will be included in the next release.

Easy State is wrapping nested store data during get-time if the immediate parent object is already observed by at least one view or autoEffect. This is done to improve performance when someone stores a lot of unobserved data inside stores somewhy.

We will remove this optimization in the next release which will fix this issue (and let us do some other cool things). It should not cause any noticeable performance changes for normal use cases.

@k1w1
Copy link
Author

k1w1 commented May 20, 2020

Great. I was thinking over the problem more last night and this is the same conclusion I came to. Thanks.

As soon as the code is in git I would be happy to test it.

@solkimicreb
Copy link
Member

This will be included in the 7.0.0 release. You can try it now with npm i @risingstack/react-easy-state@alpha, related codesandbox demo is here.

@k1w1
Copy link
Author

k1w1 commented May 21, 2020

Thanks. I have confirmed that this works in my actual application too. You are awesome.

I am curious, what change actually fixes the issue? It wasn't clear to me from the commits.

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

No branches or pull requests

2 participants