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

useSubscription hook #15022

Open
wants to merge 13 commits into
base: master
from

Conversation

@bvaughn
Copy link
Contributor

commented Mar 5, 2019

I recently shared an example useSubscription hook as a gist. Like the createSubscription class approach it was based on, this has a lot of subtle nuance– so it seems like maybe something that we should consider releasing an "official" version of (along with perhaps useFetch).

Here is the hook and some unit tests for discussion purposes.

For now I've added it inside of a new private package react-hooks (even though that name is already taken). We can bikeshed a real name later, before releasing (assuming we actually decide to do so).

Example usage:

import React, { useMemo } from "react";
import useSubscription from "./useSubscription";

// In this example, "source" is an event dispatcher (e.g. an HTMLInputElement)
// but it could be anything that emits an event and has a readable current value.
function Example({ source }) {
  // In order to avoid removing and re-adding subscriptions each time this hook is called,
  // the parameters passed to this hook should be memoized.
  const subscription = useMemo(
    () => ({
      getCurrentValue: () => source.value,
      subscribe: callback => {
        source.addEventListener("change", callback);
        return () => source.removeEventListener("change", callback);
      }
    }),

    // Re-subscribe any time our "source" changes
    // (e.g. we get a new HTMLInputElement target)
    [source]
  );

  const value = useSubscription(subscription);

  // Your rendered output here ...
}

@bvaughn bvaughn requested review from gaearon and acdlite Mar 5, 2019

@bvaughn bvaughn changed the title Use subscription useSubscription hoopk Mar 5, 2019

@bvaughn bvaughn changed the title useSubscription hoopk useSubscription hook Mar 5, 2019

@bvaughn bvaughn force-pushed the bvaughn:useSubscription branch from 9782f08 to decdf93 Mar 5, 2019

@TrySound

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

You may ask author about ownership like you did with scheduler package.
https://github.com/tj/react-hooks

});

// If the source has changed since our last render, schedule an update with its current value.
if (state.source !== source) {

This comment has been minimized.

Copy link
@Andarist

Andarist Mar 5, 2019

Contributor

isnt this somewhat a side effect in render phase? i suppose it leads to a predictable result even if a particular render gets replayed, but shouldnt generally this be done inside useEffect?

This comment has been minimized.

Copy link
@bvaughn

bvaughn Mar 5, 2019

Author Contributor

No. A side effect would be e.g. mutating a variable or calling a callback. This is just telling React to schedule some follow up work.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Mar 5, 2019

Author Contributor

This is essentially following the pattern we recommend for derived state:
https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops

This comment has been minimized.

Copy link
@Andarist

Andarist Mar 5, 2019

Contributor

Thanks for the clarification!

This comment has been minimized.

Copy link
@acdlite

acdlite Mar 5, 2019

Member

I know you showed me this earlier, but I can't remember why this part is necessary given that source is one of the deps in the effect below.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Mar 5, 2019

Author Contributor

So the thing I'm guarding against here (which maybe my comment doesn't do a good job of clarifying) is this:

  1. Subscription added to source A
  2. Component renders with a new source, source B
  3. Source A emits an update
  4. Passive effect is invoked, removing subscription from A and adding to B

We need to ignore the update from A that happens after we get a new source (but before our passive effect is fired).

I tried to make this clear with all of the inline comments but maybe I could improve them somehow?

This comment has been minimized.

Copy link
@acdlite

acdlite Mar 5, 2019

Member

Is didUnsubscribe not sufficient for that?

This comment has been minimized.

Copy link
@bvaughn

bvaughn Mar 5, 2019

Author Contributor

No. Because in the case I mentioned above, we haven't unsubscribed yet (because the passive effect wasn't yet fired).

This comment has been minimized.

Copy link
@acdlite

acdlite Mar 5, 2019

Member

We always fire pending passive effects right at the beginning of setState, to prevent this type of scenario

This comment has been minimized.

Copy link
@acdlite

acdlite Mar 5, 2019

Member

Ah but I guess that would require closing over a mutable variable. Ok now I get it.

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Yeah. If we decide to move forward with a package like this for a few derivative hooks, it may be worth reaching out to TJ about the package name. Too soon at the moment though.

// If the value hasn't changed, no update is needed.
// Return state as-is so React can bail out and avoid an unnecessary render.
const value = getCurrentValue(source);
if (prevState.value === value) {

This comment has been minimized.

Copy link
@acdlite

acdlite Mar 5, 2019

Member

This check is only necessary right after subscribing to a new source, correct? But it looks like you're checking every time the subscription produces a new value.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Mar 5, 2019

Author Contributor

This check might be useful in two scenarios:

  1. Our manual call to checkForUpdates() in the passive effect body (a few lines below).
  2. When we attach our subscription (since some sources, like rxjs, will auto-invoke a handler when attached).

I could use another local var (like didUnsubscribe) to track this but that doesn't seem any better than this check, IMO.

This comment has been minimized.

Copy link
@acdlite

acdlite Mar 5, 2019

Member

How are 1 and 2 different? I would expect that if you have 1 (the manual call) you don't need 2.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Mar 5, 2019

Author Contributor

We sync-check in case we've missed an update (1). We also need to subscribe for updates to be notified of future updates (2). We wouldn't have to sync-check if all subscription sources auto-invoked our subscription callback, but that's not the case. Some do, some don't.

@acdlite

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

It occurs to me that source serves the same function as a deps array. Have you considered using a deps API instead? Like

// Instead of `source` argument
function useStore(store) {
  return useSubscription({
    source: store,
    getCurrentValue() {
      return store.getState(),
    },
    subscribe() {
      return store.subscribe(),
    }
  });
}

// Use deps array, like we do with useEffect and useMemo
function useStore(store) {
  return useSubscription(() => {
    return {
      getCurrentValue() {
        return store.getState();
      },
      subscribe() {
        return store.subscribe();
      },
    };
  }, [store]);
}

[edit: updated to remove extra function call; now closer to useEffect] Never mind, that doesn't work

Aside from consistency between APIs, this also lets you depend on multiple values changing, and you don't need to add an extra useMemo if you want to optimize resubscribing.

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Hm. Interesting. No, I didn't really consider that variation. We wouldn't have Dan's awesome lint rule to back us up (since this is a derived hook).

@gaearon

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

I can add an exception if it's a recommended one.

@acdlite

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@gaearon Is figuring out a heuristic for custom hooks on the roadmap? Seems important. (Although I don't have any ideas.)

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Okay. I'll update this PR with the proposed API change then.

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Updated!

@bvaughn bvaughn force-pushed the bvaughn:useSubscription branch 3 times, most recently from 31af392 to 87f7c31 Mar 6, 2019

@bvaughn bvaughn requested review from acdlite and removed request for acdlite Mar 6, 2019

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Based on the outcome of our chat this morning, I've reverted the dependencies array change in favor of the previous useMemo approach. I've also updated the shared gist.

Back to you for review, @acdlite.

@bvaughn bvaughn force-pushed the bvaughn:useSubscription branch from b3e8a0a to 140cdb8 Mar 11, 2019

@gaearon

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Does the first example in the PR post need updating? I got confused for a second.

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Ah, yeah. I updated the Gist but forgot about the PR description example.

@viankakrisna

This comment has been minimized.

Copy link

commented Mar 13, 2019

subscribe: callback => {
   source.addEventListener("change", handler);
   return () => source.removeEventListener("change", handler);
}

did you mean

subscribe: handler => {
   source.addEventListener("change", handler);
   return () => source.removeEventListener("change", handler);
}

in the PR description? (callback -> handler)

return {...prevState, value};
});
};
const unsubscribe = subscribe(source, checkForUpdates);

This comment has been minimized.

Copy link
@viankakrisna

viankakrisna Mar 13, 2019

why do we need to pass source in here? wouldn't it already accessible at the call site?

export function useSelector(selector) {
  const store = useContext(ReduxContext);
  const subscription = useMemo(
    () => ({
      source: store,
      subscribe: (store, handler) => {
        // why do we need the store as argument here?
        return store.subscribe(handler);
      },
      getCurrentValue: () => {
        return selector(store.getState());
      }
    }),
    [store]
  );
  return useSubscription(subscription);
}

@bvaughn bvaughn force-pushed the bvaughn:useSubscription branch from 140cdb8 to 64f0b44 Mar 14, 2019

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

I've updated this proposal again with a less redundant API that leans more heavily on useMemo (or useCallback).

@dfahlander

This comment has been minimized.

Copy link

commented May 8, 2019

Thanks a lot @bvaughn for these answers. As a library author, I am extremely curious about any news regarding re-render mechanism of suspense (stable or unstable).

@SomeHats

This comment has been minimized.

Copy link

commented May 16, 2019

This would be a really great addition! I have a question though: should it be possible for the hook to return a value from a previous subscription source?

create-subscription uses getDerivedStateFromProps which runs before render (and then the render prop) is called. This means that if the subscription source changes, then we get the new value right away.

We don't have getDerivedStateFromProps here so useSubscription schedules a state update instead. In the meantime though, it returns the value from the previous subscription source. It seems like the type of thing that could easily lead to confusion and subtle bugs. Contrived example:

const source1 = {
  getCurrentValue: () => ({ owner: source1 }),
  subscribe: () => () => {}
};

const source2 = {
  getCurrentValue: () => ({ owner: source2 }),
  subscribe: () => () => {}
};

function App() {
  const [shouldUseSource2, setShouldUseSource2] = useState(false);
  const source = shouldUseSource2 ? source2 : source1;

  const value = useSubscription(source);
  if (value.owner !== source) {
    throw new Error("oh no");
  }

  return (
    <button onClick={() => setShouldUseSource2(!shouldUseSource2)}>
      do the thing
    </button>
  );
}

Code Sandbox link: https://codesandbox.io/embed/cool-goodall-4zuq8

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Consensus is to publish this under the package name "use-subscription". I've added the team to that package so we'll all have publish privileges. Going to rename the folder/package in this PR now.

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@SomeHats That's an interesting question you raise!

Updates queued during render are processed before work is committed. So in the case you've pointed out, the mismatched value would never make its way into the DOM. (You can confirm this by adding a layout effect that logs the DOM content.)

Although we would end up rendering the function a second time, which isn't strictly ideal, I think this should not be too problematic in practice since we don't actually commit the mismatched value. Unfortunately, I don't think we could avoid the second render in any case, since we need to update the subscription itself in state (even if we didn't update the value).

@SomeHats

This comment has been minimized.

Copy link

commented May 29, 2019

@bvaughn thanks for the response!

I'm not particularly bothered by the idea of rendering again, but I feel like it should be safe to assume in code after useSubscription that the value returned from it came from the source that was passed in.

One idea might be to change return state.value to return getCurrentValue() - do you see any potential issue with that? I'm not familiar enough to say whether that's too naive a solution.

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Calls to getCurrentValue() may involve more than just simple reads in the case of e.g. certain types of observables, so I don't think I'd want to call that method every render. Let me think on this for a few minutes. 😄

@bvaughn bvaughn force-pushed the bvaughn:useSubscription branch from cc2b9c8 to 926797b May 29, 2019

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

I think 926797b should be a safe change that avoids returning a mismatched value.

@bvaughn bvaughn requested a review from threepointone May 29, 2019

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Hey @gaearon / @acdlite / @threepointone mind taking another quick look at this? Specifically the changes in 926797b and 87bc456.

@bvaughn

This comment has been minimized.

This change seems kind of unfortunate.

@sizebot

This comment has been minimized.

Copy link

commented May 29, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

const [state, setState] = useState({
getCurrentValue,
subscribe,
value: getCurrentValue(),

This comment has been minimized.

Copy link
@SomeHats

SomeHats May 29, 2019

should this use lazy initial state to avoid calling getCurrentValue() every render as mentioned in #15022 (comment)?

This comment has been minimized.

Copy link
@bvaughn

bvaughn May 29, 2019

Author Contributor

Good call.

@threepointone
Copy link
Contributor

left a comment

I like it. Let’s discuss docs and rollout plan sometime!

"private": true,
"name": "use-subscription",
"description": "Reusable hooks",
"version": "16.8.6",

This comment has been minimized.

Copy link
@bvaughn

bvaughn May 29, 2019

Author Contributor

Something I didn't put a lot of thought into, but may be worth discussing briefly: Do we want to version-lock this package with React? Or start it off as v1?

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Note to self: I should add useDebugValue to this hook 😄

@bvaughn bvaughn referenced this pull request Jun 13, 2019

Open

Named hooks #323

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.