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

useObservable doesn't update if a change appear before the hook execution #5

Open
GuillaumeJasmin opened this issue Oct 14, 2022 · 5 comments

Comments

@GuillaumeJasmin
Copy link

GuillaumeJasmin commented Oct 14, 2022

Is this a regression?

No

Description

When a stream is updated before the subscription in the hook lifecycle, the value is not updated:

const store$ = new BehaviorSubject('A');

export function Demo() {
  useEffect(() => {
    store$.next('B');
  }, []);

  const [data] = useObservable(store$);

  return <span>{data}</span>;
}

Result: A is rendered ❌

However if the subscription appear first, it works

const store$ = new BehaviorSubject('A');

export function Demo() {
  const [data] = useObservable(store$);

  useEffect(() => {
    store$.next('B');
  }, []);

  return <span>{data}</span>;
}

Result: B is rendered ✅ .

I spent a lot of time to understand why my data was not updated, and I finally spot this difference 🤯 .
After a look at the code, I understood why: the subscribe() is done inside a useEffect, and so executed after the first useEffect, but we never recheck the value of the first emission inside the subscribe.

To fix it, we need to ensure that the value has not changed immediately after the subscription:

subscription.current = sourceRef.subscribe({
  next(value) {
    if (emitsInitialSyncValue && firstEmission) {
      firstEmission = false;
+      if (value !== nextValue.current) {
+        nextValue.current = value;
+        forceUpdate();
+      }
    } else {
      nextValue.current = value;
      forceUpdate();
    }
  },
  error: setError,
  complete: setCompleted.bind(null, true),
});

What do you think ?

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

No response

Please provide the environment you discovered this bug in

No response

Anything else?

No response

Do you want to create a pull request?

No

@NetanelBasal
Copy link
Member

Can you explain why you are doing this? Why not start with B? :)

@GuillaumeJasmin
Copy link
Author

It's just an example... Of course I never do this kind of code.

If you need a real use case, imagine things like this (with elf)

export function Demo() {
  const store$ = useStore();

  useEffect(() => {
    const userId = localStorage.getItem('userId');
    
    if (userId) {
      store$.update((prevState) => ({ ...prevState, userId  }));
    } else {
      // ... fetch user id
    }
  }, [store$]);

  const [data] = useObservable(store$);

  return <span>{data.userId}</span>;
}

This doesn't work ❌ last render has userId: undefined .

Once again, it's a simple example with only 1 component.

Imagine a more hidden behavior like this:

export function Demo() {
  useInitAuthentication();
  const { userId } = useUserId();

  return <span>{userId}</span>;
}

⬆️
This doesn't work ❌

export function Demo() {
  const { userId } = useUserId();
  useInitAuthentication();

  return <span>{userId}</span>;
}

⬆️
This works ✅

🤯 It's impossible to know the correct order to use. The last render should always produce the same result.

When the store is updated synchronously after the 1rst render, the component is never re-render with up to date value.

--

If you still wants another exemple, the same code works with useSelector of react-redux:

function Demo() {
  const dispatch = useDispatch()

  useEffect(() => {
    const userId = localStorage.getItem('userId');
    
    if (userId) {
      dispatch(actions.setUserId(userId));
    } else {
      // ... fetch user id
    }
  }, [dispatch]);
  
  const userId = useSelector(state => state.userId);
  
  return <span>{userId}</span>;
}

This will always works, whatever the order call of the hooks. Howerver it's doesn't work with useObservable .

So my proposal is to check the value just after the subscription, because the store could have been updated just before the subscription. Is it understandable ?

Also, congratulation for efl package 🎉 it's very powerful. I made the same kind of store few years ago with a BehaviorSubject, but I have switch to elf now.

@NetanelBasal
Copy link
Member

Yes. Do you want to create a PR?

@GuillaumeJasmin
Copy link
Author

Yes I will do that the next week, thanks 🙂

@NetanelBasal
Copy link
Member

We can change the code and use the syncExternalStore hook.

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