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

RFC: React Hooks #68

Merged
merged 3 commits into from Nov 21, 2018
Merged

RFC: React Hooks #68

merged 3 commits into from Nov 21, 2018

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Oct 25, 2018

In this RFC, we propose introducing Hooks to React. See the RFC and the documentation for more details.

https://reactjs.org/docs/hooks-overview.html

View formatted RFC

(The talk video has now been published too.)


Nov 16 Update by @sophiebits:

See @sebmarkbage’s comment #68 (comment) for a detailed response to the preceding comments that covers various tradeoffs and articulates why we’ve landed on the current design. If you have something to say, please check that it is not answered there already.

In this RFC, we propose introducing *Hooks* to React. See the RFC and the documentation for more details.

https://reactjs.org/docs/hooks-overview.html
@grabbou
Copy link

grabbou commented Oct 25, 2018

Question about persisting values of useState - is the preferred way to create a custom usePersistedState hook that calls into storage mechanism?

I was wondering if there was a way to batch the write operations in environments such as React Native and how did you approach that problem at Facebook during your initial adoption?

@pcmaffey
Copy link

Is this the right place for feedback? First of all - amazing work. I love where this is going with composability. My big question is about performance of Hooks.

Are all these function hooks redefined and recalled on every render?

@pheuter
Copy link

pheuter commented Oct 25, 2018

Just watched the React Conf keynote, awesome presentation, super excited to give hooks a shot!

Not sure if this is the right place to ask but was wondering how function components that make use of hooks affect server-side rendering. In useEffects() for example, will errors be thrown when accessing browser apis like window during ssr?

@pheuter
Copy link

pheuter commented Oct 25, 2018

@pcmaffey Looks like this FAQ entry addresses your performance concern.

@JoshuaKGoldberg
Copy link

JoshuaKGoldberg commented Oct 25, 2018

Very exciting, this looks awesome!

Questioning the tuple return type:

const [count, setCount] = useState(0);

Why an array tuple instead of an object containing count and setCount members? Is this for performance (and if so, do we have numbers to show browsers or Node behaving strongly differently)? It feels like leaving the returned object open to adding more fields might make it easier to remain backwards compatible in the future, similar to how APIs often end up taking in single objects as a parameter rather than long lists of parameters.

const { value, set } = useState(0);

Edit: yes, per @jaredLunde's comment, the properties would need constant names.

@jaredLunde
Copy link

jaredLunde commented Oct 25, 2018

@JoshuaKGoldberg How would you customize those object properties? Array is the cleanest way it works with custom naming.

@alexeyraspopov
Copy link

alexeyraspopov commented Oct 25, 2018

Why an array tuple instead of an object containing count and setCount members?

You can assign names you need. Object destructuring requires you to put specific keys. You may end up having something like

let { state: name, setState: setName } = useState('Name');
let { state: surname, setState: setSurname } = useState('Surname');

Which is not the case with tuples.

@jamesplease
Copy link

jamesplease commented Oct 25, 2018

I'm still familiarizing myself with this API, but I'm optimistic about these changes. Great work, React team!

My main concern with hooks is related to the learning curve. There are some APIs introduced that new (and seasoned) developers may not immediately pick up on.

One in particular that stands out the most to me is useEffect(myEffect, []). It's not very expressive. For those who haven't read the docs, what do you think this does? I'd be surprised if someone could guess.

For those who are still familiarizing themselves with the API, it calls the effect only on mounting and unmounting, and not on updates (docs here).

I know that this proposal introduces a lot of new functions, but it might be worthwhile adding another one that functions the same as useEffect(myEffect, []). As for the name? I'm not sure. Perhaps something like useMountEffect()?

@eps1lon
Copy link
Contributor

eps1lon commented Oct 25, 2018

It is a very early time for Hooks, so some integrations like DevTools support or Flow/TypeScript typings may not be ready yet.

Will I be able to inspect what hooks a component is using?

Having an object with named properties as state made it easy to see what's what. I guess this will be hard with the new useState?

@benjamn
Copy link

benjamn commented Oct 25, 2018

@eps1lon According to the FAQ, useState is backed by a list of anonymous memory cells:

There is an internal list of “memory cells” associated with each component. They’re just JavaScript objects where we can put some data. When you call a Hook like useState(), it reads the current cell (or initializes it during the first render), and then moves the pointer to the next one. This is how multiple useState() calls each get independent local state.

So if you ever find yourself trying to debug the underlying state, you're probably going to be working with a linked list of unnamed values. To figure out which memory cell is which, you'll have to refer back to the order of your useState calls.

Full disclosure: I'm planning to open an issue today to discuss a system for providing meaningful names for your useState calls, which should significantly relax the current usage restrictions, and eliminate the need for a linter to enforce those rules.

@megamaddu
Copy link

megamaddu commented Oct 25, 2018

Positional effects (using any of these use* functions is an effect, even just to read state) worry me. It seems easy to get wrong, and doing so might not present until production. For the same reason it also seems like it'll be extremely difficult to introduce to beginners -- "the magic function works here but not there".

Using Symbols instead of positional keys would make it easier to understand and harder to get wrong:

const $count = Symbol("count");

function MyComponent() {
  const [count, setCount] = useState($count, 0);
  ...
}

Edit for clarification: I don't mean to say the Symbol would be a global key -- it would still use the current context (the Fiber, I assume) as the primary key and the Symbol as the secondary key (where the effectful call order is currently being used).

Another edit for followup: After playing with hooks a bit more I don't feel as strongly about this as I thought I would. It's not perfect, but you could also pass the wrong Symbol in this alternative as well. Also seeing the eslint plugin catching on relieves the worry I had a bit too.

@adamhaile
Copy link

Just curious, are React hooks at all inspired by S and Surplus? There are similarities down to even the names and terminology used. S has S.effect() and S.cleanup(), and useState() is analogous to S's S.data().

For instance, here's the hooks example in Surplus:

function Example() {
  const count = S.data(0); // S.data instead of React.useState

  S.effect(() => { // S.effect instead of React.useEffect
    document.title = `You clicked ${count()} times`;
  });

  return (
    <div>
      <p>You clicked {count()} times</p>
      <button onClick={() => count(count() + 1)}>
        Click me
      </button>
    </div>
  );
}

In fact, I used almost this exact same example in the Surplus FAQ to explain how functional components could have state -- the very same issue hooks are solving for React.

I'm the author of S and Surplus, and reading the hook docs gave me a strong case of déjà vu! The similarities may be coincidental -- we're probably reading the same upstream sources. But if there was any influence, I thought that would be cool :).

@alexeyraspopov
Copy link

alexeyraspopov commented Oct 25, 2018

@alqamabinsadiq, sorry if my comment was somehow confusing. This is not how the hook intended to be used, I was making an example of additional effort required for using object destructuring. This is how the correct code looks like:

let [name, setName] = useState('Name');
let [surname, setSurname] = useState('Surname');

Less effort to write, less effort to read.

@sophiebits
Copy link
Member

@adamhaile To my knowledge we weren’t influenced at all by Surplus (this is my first time seeing it). It’s true the “effect” name is similar, but both names are descriptive, so I guess it shouldn’t be too surprising that the names overlap.

@sebmarkbage
Copy link
Collaborator Author

@benjamn @spicydonuts If you decide to publish a follow up RFC with named hooks which doesn’t have the conditionality constraints, then be sure to cover other pitfalls that you may also get. In the current proposal there is clearly one thing that will unexpectedly break when you don’t have names. However there are other things that might become confusing too. Such as that an effect doesn’t cleanup and refire when it’s in a conditional. So even with names you might find it best practice to avoid conditionals.

@megamaddu
Copy link

I see what you mean. Hmm.. that's unfortunate 🤔

@ghost
Copy link

ghost commented Oct 25, 2018

I like the idea, but I think the naming doesn't really reflect what's going on here. The term hook in other contexts, such as web hooks or Emacs, implies that something happens at a particular point in time and calls out to some other functions/methods to make it happen.

In the words of the Github docs for Web hooks:

Webhooks allow you to build or set up GitHub Apps which subscribe to certain events on GitHub.com. When one of those events is triggered, we'll send a HTTP POST payload to the webhook's configured URL.

For example web hooks are called whenever Github detects a branch has been created. They're called in Emacs whenever a file is loaded or a particular mode (for syntax highlighting + shortcuts) is activated.

In React, the only hooks I can think of at the moment are the componentDidMount and componentWillUnmount functions, since they're part of a lifecycle and call out to some other function (as part of being overridden in a Component class)

From the React Hooks documentation it looks like these are managers:

  • useState manages and coordinates state
  • useEffect manages and supervises side-effects
  • useContext manages context

Other options for naming:

  • manager
  • supervisor
  • coordinator
  • handler

I'm sure there are other, better names other than "hook" which can be used.

Also, the naming of the methods don't need to change since none of them seem to have the word "hook" in them, the naming change would just affect the rest of the documentation/examples.

@leebyron
Copy link

Loving this new API, but I have one primary concern - where the hooks come from.

I'm concerned that these hook functions are provided by the top level react module and imply (or require) global state to work. Ideally, hooks and effects functions could be provided to the functional component when it's executed.

So while this example as illustrated by Dan requires from the module:

const { useContext } = require('react')
const SomeContext = require('./SomeContext)

function Example({ someProp }) {
  const contextValue = useContext(SomeContext)
  return <div>{someProp}{contextValue}</div>
}

Was it considered to pass the hooks into the functional component?

Perhaps looking something like:

const SomeContext = require('./SomeContext)

function Example({ someProp }, hooks) {
  const contextValue = hooks.useContext(SomeContext)
  return <div>{someProp}{contextValue}</div>
}

This means functional components which use hooks are (Props, Hooks) => ReactNode. While this is still not "pure" because hooks have effects and are not pure, at least the renderer could provide the appropriate hooks per call-site instead of relying on global state. I could imagine that the Fiber renderer might have different implementation of hooks than some potential future renderer, a server-side renderer, a test renderer with mocked effects, etc.

Thoughts?

@kartikag01
Copy link

for new react developers, useState hooks will be a magic.

@ntucker
Copy link

ntucker commented Oct 25, 2018

How does this interact with React.memo()? Does it just shallow check props and ignore state and context. If not, how does it know what state and context to check?

@alexeyraspopov
Copy link

I'm concerned that these hook functions are provided by the top level react module and imply (or require) global state to work. Ideally, hooks and effects functions could be provided to the functional component when it's executed.

@leebyron, I think, the way how it works, is by using an incapsulated access to the current rendering fiber that holds the state and effects. The API may look like something "global" happens under the hood, but it is still scoped to the specific fiber that reflects the component that is rendering at the moment.

@benjamn
Copy link

benjamn commented Oct 25, 2018

@leebyron Alternatively, functional components could be invoked by the React internals with a meaningful this object, containing any number of useful methods.

@david
Copy link

david commented Oct 25, 2018

What's the advantage of this approach over a HOC-based one such as the one used, for example, in recompose?

@sebmarkbage
Copy link
Collaborator Author

@leebyron Conceptually they’re algebraic effects. It just happens that the engine (a pointer to the current “handler”) lives in the react package. Note that the actual implementation doesn’t live in the react-package. So anyone can implement their own dispatcher. Even inside a react render. If this is a popular pattern you could just have a general purpose pattern for this like “dispatcher” package or something.

Just like context there doesn’t have to be one. In multi-threaded environments there can be several.

We considered passing it but then you would have to pass it through multiple layers of indirection. We already know this to be a pain (which is why we have context to begin with) and for something as light weight as custom hooks it is even more so.

@jaredLunde
Copy link

@david acdlite/recompose@7867de6

@sebmarkbage
Copy link
Collaborator Author

@benjamn The this was actually in the first idea and later dropped because it added weirdness and got even weirder when composed with custom hooks. Since they would need to change to a different variable or use .call.

@quantizor
Copy link

Are there any concerns about the interaction of transpilers with state hooks? The order of function calls could conceivably be rearranged by a transpiler or minifier in a way that differs between an SSR and client-side bundle.

@gaearon
Copy link
Member

gaearon commented Mar 29, 2019

@timkendrick

In your proposed design, props becomes an argument of useEffect callback. Perhaps I misunderstood how that works. But it seems like this breaks encapsulation quite a bit because any custom Hook now suddenly has access to its owner component's props. That makes them non-compositional. One Hook could read props (assuming specific ones exist), and then later you might change the component to not receive that prop. Or you might lift a Hook out of a component, and the Hook will break. There's no obvious way that you'd know to update the custom Hook to not depend on it. (I talk a bit about spooky action at distance like this here.)

It's also not entirely clear to me how you'd do something like this:

function useCustomThing(value) {
  useEffect(() => {
    doSomethingWith(value)
  })
}

function MyComponent({ prop }) {
  let [state, setState] = useState('')
  // ...
  let derived = useMemo(() => expensiveStuff(prop, state), [prop, state])
  useCustomThing(derived)
  // ...
}

In other words, it's not just "props and state" that need to be captured in effects and other closures, but anything that's calculated from them. Custom Hooks need access to the current values as their arguments — not just initial values. For example I don't have a clear idea of how you'd write this useFriendStatus Hook where friendID is an argument, and the Hook doesn't make assumptions that it comes from props.id (because custom Hooks aren't supposed to be aware of component's props).

Additionally, Hooks should be able to chain. So you should be able to do

const x = useStuff(props.foo)
const y = useStuff(x)
const z = useStuff(y)

If useStuff internally needs its argument inside an effect, it can't just do props => props.foo anyway. (Even if that was compositional — which it is not.)

Your proposal about accessing at "wrong time" also doesn't solve the core of the issue. It's not that there is a "right" and "wrong" time to access it. It's that we want closures to capture the values we rendered with, and to keep "seeing" those values forever. That's really important for concurrent mode where a notion of current value doesn't really exist. Hooks design models a component as being in many non-clashing states at the same time, instead of switching the "current" state (which is what classes model well). People don't really need to think about these details, but they're motivating the design a lot.


One thing that slightly irks me is your assumption that we didn't want to really take this feedback into account, or that we just decided to ignore the discussion. Or that we didn't consider this design.

I understand it might seem this way because my comment responding to your specific proposal wasn't very long. But as we mentioned earlier, realistically responding to every single proposal in detail would be a full-time job. I tried my best in this blog post but there's only so much you can document, and people will still come up with slightly different combinations. I'm sorry if our responses haven't been sufficient to satisfy your curiosity. Up until the release day, we read all the proposals, even if we didn't respond to all of them.

I hope my questions above shine some light on why this particular variation didn't make it through. I will try my best to respond to follow-ups on this thread but I hope you can also understand that five months later after publishing the proposal, and given we also thought about it for several months before implementing (including this variation), responding to new slight variations that suffer from the same issues as older variations is not a top priority. (Thanks a lot for the bug report though!)

@timkendrick
Copy link

timkendrick commented Mar 29, 2019

Thanks @gaearon – you're totally right, I was relying all the (props) => … functions because the statically-defined hooks need some way of accessing the current props, but I completely overlooked the fact that this breaks encapsulation in custom hooks. I take your point on the 'chainability' as well. As far as I'm aware, these hadn't come up in previous criticisms of a factory approach so I thought I was coming up with a novel solution, but it appears you've already addressed this – I can only apologise for coming on so strong about that suggestion, I was only considering the 'capturing' criticism and didn't spend enough time thinking through the composablility side of things.

Just as a side note, on the capturing side, I think perhaps you're misunderstanding my motivation for preventing access at the "wrong time" – maybe I'm way off here, but in my mind preventing accessing variables at the "wrong time" effectively forces the user to immediately capture any variables they plan on using within the hook body, by only allowing them to access the values synchronously ('right at the start of the function'). This means they're forced to "see" those values forever, as you suggest (seeing as any later attempts to read the values would fail with an error). The error throwing is therefore a mechanism that forces users to perform this capturing of immutable values. It could well be me that's misunderstanding your comment though.


Your response has made me think about the tone I've been taking throughout this thread: I owe you and the rest of the team a big apology for throwing round accusations about not listening to users. It must be very frustrating to see people keep coming up with suggestions you've already considered and discarded.

I suspect this is likely happening because (perhaps unusually for this project?) the design process happened behind closed doors, for reasons you discussed in your Reddit comment. This runs counter to most RFC processes I've been involved with, where somebody proposes an idea (perhaps with an initial strawman implementation to guide discussion), then various people chip in with criticisms, suggestions and alternatives, the merits of which are discussed in the RFC thread as the proposed solution gradually mutates into its final state, all alternatives having been discussed at various points throughout the thread. With the hooks API, the first stages of this process seem to have happened internally within Facebook, so for those of us reading this RFC we don't have any knowledge of the rejected prior attempts, so we have no idea that you've already discussed these alternatives at length and already found the flaws in them. If you'd dumped a bunch of gists/sketches of rejected alternatives to accompany the PR (to 'bring the spectators up to speed') there might have been fewer people like me pestering you with the same flawed proposals that you've already examined – although I do realise this involves effort on your part to placate a small minority of people who have problems with the API. If this had just been a feature announcement I would have just moaned a bit to my coder friends and moved on – it was the "RFC" positioning of it that made me expect more of a two-way debate, but that might be my misinterpretation more than anything, and I get that you're too busy keeping the ship afloat to respond in depth to every suggestion.

The reason I got so deeply involved in this thread is that I care intensely about the way React is headed – in large part because React is currently my entire career: I market myself specifically as a 'React Developer' nowadays because I love using it so much that I wouldn't consider a job using any other framework. I still want to be able to call myself a React developer in 5 years' time, and so I really hope that between now and then nothing happens to discourage new users. I've had conversations with various tech decision-makers (mainly from imperative backgrounds) who already struggle to understand the concepts behind React even without hooks, and I hate to think of somebody one day rejecting React for use in a new project on account of it being 'too weird'. Regardless, I'm sure the project is in excellent hands: this is the first API decision I've ever disagreed with in the five years I've been using React, so I'm definitely optimistic in the long run.

In short: I'm sorry for letting my exasperation get the better of me; you're all doing an incredible job, despite people like me making that a very difficult job at times. Hooks are a real game-changer – let's hope that when I'm still a React Developer 5 years from now there's even more cool features that make me love coming into work in the morning 😉

@mindplay-dk
Copy link

@gaearon would you comment on this approach?

It's not nearly as complete (or pretty) as what @timkendrick posted, but I think I've worked through the most essential parts: state, effects, layout-effects, context and refs, with basic examples of each.

What is the problem or what limitations do you see with this approach?

Maybe point to something tricky you can do with hooks, and I'll see if I can reproduce it with this?

@timkendrick
Copy link

timkendrick commented Mar 29, 2019

@mindplay-dk it's almost certainly a better approach than my suggestion, but the obvious drawback to me is that you can't depend on prop values in e.g. the useEffect() handler.

For example, consider the archetypal effect example:

const StatusFeed = component((instance) => {
  instance.useEffect(() => {
    api.subscribeToFriendStatus(props.friendID, ...);
    return () => {
      api.unsubscribeFromFriendStatus(props.friendID, ...);
    };
  });
  return (props) => ...;
});

The subscription mechanism requires access to current props in order to determine the correct ID to fetch, however in your approach you don't have access to props during the hook creation phase.

My first thought to try to work around this would be to store the friendID in some intermediary hook in order to access it within the effect:

const StatusFeed = component((instance) => {
  const [friendState] = instance.useState(({ friendID }) => ({ friendID }));
  instance.useEffect(() => {
    api.subscribeToFriendStatus(friendState.friendID, ...);
    return () => {
      api.unsubscribeFromFriendStatus(friendState.friendID, ...);
    };
  });
  return (props) => ...;
});

…this feels a little clumsy due to having to introduce a composite mutable state object just to track a primitive value, but it also doesn't actually solve the problem, seeing as we want the effect to automatically re-run every time the prop changes.

Maybe we could introduce some kind of useProps() hook that tracks the current state of the props and can be used as a dependency of useEffect:

const StatusFeed = component((instance) => {
  const friendState = instance.useProps(({ friendID }) => friendID);
  instance.useEffect(
    () => {
      api.subscribeToFriendStatus(friendState.current, ...);
      return () => {
        api.unsubscribeFromFriendStatus(friendState.current, ...);
      };
    },
    [friendState.current] // This won't work
  );
});

…as well as the 'encapsulation-breaking' properties of any potential useProps() hook (raised by @gaearon's response to my most recent suggestion), this would of course not work either, seeing as your approach relies on the mutability of the friendState object returned by the hook. Presumably the factory only runs once, so the dependencies array is incorrect, so this would work for the initial run but would not re-run on subsequent renders if the component is re-rendered with a different props.friendID.

I think this exposes a more general problem with effects/callbacks whose implementations depend on the current values of other hooks: how do you write the dependencies array for a useEffect whose callback depends on another hook's current value? One option would be to have the dependencies array be a thunk that you call to get the current values:

const StatusFeed = component((instance) => {
  const friendState = instance.useProps(({ friendID }) => friendID);
  instance.useEffect(
    () => {
      api.subscribeToFriendStatus(friendState.current);
      return () => {
        api.unsubscribeFromFriendStatus(friendState.current);
      };
    },
    () => [friendState.current]
  );
  return (props) => ...;
});

While this might get round the dependencies array issue, it still relies on the encapsulation-breaking useProps hook, and I'm worried that any approach that relies on a mutable .current property (or the equivalent mutable .count property in your counter.useState() demo) would still be susceptible the 'capturing' concerns raised by @gaearon a while back.

As for whether it's a good idea to capture the relevant prop state as a separate hook value in the first place, I actually quite like this, seeing as to my mind it fairly accurately models the fact that the current state is derived from the props (and in retrospect I probably should have used a useProps hook in my suggestion rather than relying on (props) => … functions), however critics might see it as redundant?

Anyway, this is just some stuff to think about – and there's also a good chance I'm misunderstanding your API, so take it with a pinch of salt 😃

@danielkcz
Copy link

danielkcz commented Mar 29, 2019

@mindplay-dk You can perhaps go through A Complete Guide to useEffect which depicts thought process behind this in a brilliant way. Try to apply it to your approach if you can achieve similar results.

@mindplay-dk
Copy link

@timkendrick this is great feedback, thanks :-)

Did you understand that what gets passed to your component() callback is basically just a regular Component instance? You should be able to to just access it's props without any further ado:

const StatusFeed = component((instance) => {
  instance.useEffect(() => {
    api.subscribeToFriendStatus(instance.props.friendID, ...);
    return () => {
      api.unsubscribeFromFriendStatus(instance.props.friendID, ...);
    };
  });
  return (props) => ...;
});

I guess I could pass them to the useEffect() callbacks for convenience:

const StatusFeed = component((instance) => {
  instance.useEffect(({ friendID }) => {
    api.subscribeToFriendStatus(friendID, ...);
    return ({ friendID }) => {
      api.unsubscribeFromFriendStatus(friendID, ...);
    };
  });
  return (props) => ...;
});

I'm sort of leaning towards the philosophy of keeping it "just a component" though, and it's perhaps a bit confusing with the friendID in the inner scope hiding the friendID from the outer scope? And as said, you have access to instance.props already, so probably not necessary?

@timkendrick
Copy link

timkendrick commented Mar 29, 2019

Oh sorry, I didn't realise it was an actual instance – I assumed it was some kind of context object which you could use almost like a 'placeholder' reference to the component, gotcha.

Is there even such a thing as a component instance with non-class-based-components? I thought that in the world of modern function components, you just have the combination of render function plus a collection of 'memory cells' within the current fiber that store hook state etc, and no actual instance? As far as I was aware the current thinking with the fiber internals is to move away from long-lived component instances towards a more stateless model that presumably is a better fit for async rendering / increased parallelization etc… I could be way off on this though.

Either way, I think it's safe to say I misunderstood your API so my comments don't really apply to it 😃

@mindplay-dk
Copy link

mindplay-dk commented Apr 1, 2019

@timkendrick I'm less interested in the micro performance details. What I like about hooks is how they enable you to compose behavior - I don't have any problem with class-based components as such (performance or otherwise) I'm just looking for a practical way to compose components and reuse behaviors and states. My main motivation is there are clearly initialization and rendering phases - I'd like to explore approaches that don't try to force them into a single phase.

I don't think being "stateless" is even a goal (since there is clearly a component state, whether it's stored in a component instance or somewhere else) and I'd like to explore approaches that model that fact, rather than trying to make the API look functional by relying on global state, side-effects, counting calls, etc.

I like simple things, that's all :-)

@zeg-io
Copy link

zeg-io commented May 15, 2019

I completely agree with the proposal here: facebook/react#14007 the text of which is below. I didn't see this pushed here as suggested, but the idea of useHooks passing an isMounted flag is a great one and I didn't want to see it just die. To quote from the post from @wesleycho:

Do you want to request a feature or report a bug?

Feature request? This is more to kickstart a discussion.

What is the current behavior?

With the current proposal, with any promise-based function call, useEffect must be used something like so

function MyComponent(props) {
  const [state, setState] = useState(0);
  let unmounted = false;
  useEffect(() => {
    myXhrRequest()
      .then(data => {
        if (!unmounted) {
          setState(data);
        }
      });
    return () => {
      unmounted = true;
    };
  });
  ...
}

This is a fairly awkward pattern for promises, as one needs to add an extra check for mounted state potentially quite divorced from the unmount logic. It should be noted that for subscription-based patterns like with rxjs this is fine, but for promises this stinks and is a bit confusing - this would also get repetitive if there are multiple requests involved.

Ideally the useEffect should make it easier to clean up asynchronous code without imposing on the pattern a user would to carry out async behavior. One manner that could remove the need for the confusing return function is to provide a isMounted helper function as an argument for the function passed into useEffect that returns whether the current component is mounted. It does not solve the verbosity completely, which might be outside the scope of React, but it removes the need for extra boilerplate that arguably hurts the readability.

With this proposal, the useEffect part could be rewritten like this

  useEffect(({ isMounted }) => {
    myXhrRequest()
      .then(data => {
        if (isMounted()) {
          setState(data);
        }
      });
  });

Just some thoughts anyhow, I would understand if this proposal is rejected.

@jwipeout
Copy link

I really like the change. It makes for more clear and concise code. I have been trying to see an example of how to replace redux using context and hooks. Can you point me in the right direction? I have read through many articles and looks like everyone is doing it a little different and it is difficult to know, which one is better.

@jakewtaylor
Copy link

jakewtaylor commented May 28, 2019

@zeg-io Couldn't you just make that simple hook yourself and reuse it?

const useMountedEffect = (callback) => {
    let mounted = true;
    
    useEffect(() => {
        callback({ mounted });
        
        return () => mounted = false;
    });
};

// Use:
useMountedEffect(({ mounted }) => {
    myXhrRequest()
        .then(data => {
            if (mounted) {
                setState(data);
            }
        });
});

You could even adjust it so that the callback is only ever called if mounted, like:

useEffect(() => {
    if (mounted) {
        callback();
    }

    return () => mounted = false;
});

@c708423
Copy link

c708423 commented Sep 4, 2019

@zeg-io I think we can cancel the request in useEffect return in case cause a memory leak error msg.
If u just want to store state as a component state.

useEffect(() => {
  const myRequest = myXhrRequest()
    .then(data => {
        setState(data);
    });
  return () => {
    myRequest.cancel();
  }
});

If this a reuse state. cache it, or something like global store.

useEffect(() => {
  const myRequest = myXhrRequest()
    .then(data => {
      dispatch('updateSomeData', data);
      // or globalStore.yourData = data;
    });
  return () => {
    // no need to cancel
  }
});

@lukcad
Copy link

lukcad commented Sep 15, 2019

Loving this new API, but I have one primary concern - where the hooks come from.

I'm concerned that these hook functions are provided by the top level react module and imply (or require) global state to work. Ideally, hooks and effects functions could be provided to the functional component when it's executed.

So while this example as illustrated by Dan requires from the module:

const { useContext } = require('react')
const SomeContext = require('./SomeContext)

function Example({ someProp }) {
  const contextValue = useContext(SomeContext)
  return <div>{someProp}{contextValue}</div>
}

Was it considered to pass the hooks into the functional component?

Perhaps looking something like:

const SomeContext = require('./SomeContext)

function Example({ someProp }, hooks) {
  const contextValue = hooks.useContext(SomeContext)
  return <div>{someProp}{contextValue}</div>
}

This means functional components which use hooks are (Props, Hooks) => ReactNode. While this is still not "pure" because hooks have effects and are not pure, at least the renderer could provide the appropriate hooks per call-site instead of relying on global state. I could imagine that the Fiber renderer might have different implementation of hooks than some potential future renderer, a server-side renderer, a test renderer with mocked effects, etc.

Thoughts?

// THIS IS FUNCTION BY HOOK
function Example(){
  const [count,setCount] = React.useState(0);
  return(
    <div>
      <b>You clicked this button {count} times.</b> <br />
      <button onClick={() => setCount(count + 1)} > Click me </button>
    </div>
  );
}

@czf1998
Copy link

czf1998 commented Apr 3, 2020

what about this? @pkf1994

@ranneyd
Copy link

ranneyd commented Apr 10, 2020

@zeg-io Couldn't you just make that simple hook yourself and reuse it?

const useMountedEffect = (callback) => {
    let mounted = true;
    
    useEffect(() => {
        callback({ mounted });
        
        return () => mounted = false;
    });
};

// Use:
useMountedEffect(({ mounted }) => {
    myXhrRequest()
        .then(data => {
            if (mounted) {
                setState(data);
            }
        });
});

You could even adjust it so that the callback is only ever called if mounted, like:

useEffect(() => {
    if (mounted) {
        callback();
    }

    return () => mounted = false;
});

One issue is your useEffect does pass [] as the second param, meaning it will run on every re-render, not just when the component unmounts.

This is an issue I've noticed with the "bailout" pattern: I may have a useEffect that depends on several variables. I only want it to skip the state setting when the component is unmounted, not every time that particular useEffect invocation is cleaned up. For instance, I may have a useEffect that refetches data based on user input. However, instead of overwriting the data, it saves it in a keyed object so if they set the settings back, we don't need to refetch. If they change the settings while we're still fetching, I don't want to throw away the results, so I can't use the cleanup function to signify that state change is illegal.

I've done something similar but with a custom ref-based hook:

export const useIsMounted = () => {
  const isMountedRef = useRef(true);
  useEffect(() => {
    return () => {
      isMountedRef.current = false;
    };
  }, []);
  const getIsMounted = useCallback(() => isMountedRef.current, []);
  return getIsMounted;
};

What's cool about this is:

  • getIsMounted is from a useCallback with [] as the second arg, meaning it will never get re-defined. We can safely pass it as an dependency in a useEffect without fear that it will, itself, cause the useEffect to re-run
  • isMountedRef is a ref as opposed to a state variable, so getIsMounted doesn't need to specify it as a dependency
  • The useEffect has [] as the dependencies, meaning it will only run on mount, meaning the cleanup will only run on unmount.

When using this, you use const getIsMounted = useIsMounted(); then call getIsMounted(). Again, it needs to be the function, not the value, so you're getting the value out of the ref and not forcing useEffect to rerun.

@jsamr
Copy link

jsamr commented Sep 11, 2020

I have a feeling that useEffect is missing an optional third argument, which are the dependencies required to change for the effect to be run. The second "deps" argument might, or might not coincide with the aforementioned argument, depending on component behavior requirements, and sometimes the intersection between those two could be empty. A basic example: I want to scroll to top of a component when the content changes (first dependency), but this effect also depends on a variable padding top (second dep). Currently what I can do is:

function MyComponent(props) {
  const { paddingTop, content } = props;
  const ref = React.useRef();
  React.useEffect(() => {
    // scroll to paddingTop when content changes?
    ref.current.scrollTo(0, paddingTop);
  }, [paddingTop, content]);
 return <div ref={ref}>...</div>
}

There is an undesired behavior: the hook is executed on paddingTop changes. Moreover, "content" is not, semantically, a dependency of the callback, but rather a dependency of the "when this side effect should happen". So I could use a ref, store the previous value of paddingTop, and compare the two. But that is cumbersome. What I would like to do, is express the "when this side-effect should happen" dependencies declaratively:

function MyComponent(props) {
  const { paddingTop, content } = props;
  const ref = React.useRef();
  React.useEffect(() => {
    // scroll to paddingTop when content changes.
    ref.current.scrollTo(0, paddingTop);
  }, [paddingTop], [content]);
 return <div ref={ref}>...</div>
}

So a new signature for useEffect would be:

/**
 * @param what - what this side effect does?
 * @param whatDeps - which variables modify what the side effect does?
 * @param whenDeps - which variables modify when the side effect occurs?
 */
type useEffect = (
  what: (...args: any[]) => any,
  whatDeps: any[],
  whenDeps?: any[]
) => void;

The community seems to be in need of a solution, see https://stackoverflow.com/q/55724642/2779871 What are your thoughts?

EDIT-1: added signature.
EDIT-2: Comment here #176

@gaearon
Copy link
Member

gaearon commented Sep 11, 2020

If you have ideas, concerns or suggestions, it would be best to file a new issue in the React repo or a new pull request in this repo. We're not tracking old discussions, and this one in particular will email 300 people on every comment.

@MingChe0102
Copy link

hook函数 与 hoc 使用的时机是什么时候?有什么区别和意义?

@workspaceking
Copy link

workspaceking commented Apr 4, 2021

Very exciting, this looks awesome!

Questioning the tuple return type:

const [count, setCount] = useState(0);

Why an array tuple instead of an object containing count and setCount members? Is this for performance (and if so, do we have numbers to show browsers or Node behaving strongly differently)? It feels like leaving the returned object open to adding more fields might make it easier to remain backwards compatible in the future, similar to how APIs often end up taking in single objects as a parameter rather than long lists of parameters.

const { value, set } = useState(0);

Edit: yes, per @jaredLunde's comment, the properties would need constant names.

For React it's easy to get the state and set function from an array, in the case of an object you have to extract keys using Object.keys() then spread the state and set function.

In the case of an array index, 0 is the state and 1 is the function that handles the change in state

Copy link

@alsarem-official alsarem-official left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahmed

@omeid
Copy link

omeid commented Jan 31, 2022

For the better or worst, this is a dead horse. Would one of the maintainers please lock this issue or at least restricted to avoid all the spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment