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

Add instance parameter for Hooks API #138

Closed
wants to merge 1 commit into from
Closed

Add instance parameter for Hooks API #138

wants to merge 1 commit into from

Conversation

0e39bf7b
Copy link

@0e39bf7b 0e39bf7b commented Jan 7, 2020

PR for this issue facebook/react#17742

@facebook-github-bot
Copy link
Collaborator

Hi 0e39bf7b! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link
Collaborator

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@0e39bf7b
Copy link
Author

0e39bf7b commented Jan 9, 2020

@j-f1 @flaviendelangle if it's possible would you please explain a bit more your dislikes? Do you think that current behavior is better?

@j-f1
Copy link

j-f1 commented Jan 9, 2020

Do you think that current behavior is better?

Yes. Since the current API works just fine. Changing this would require re-teaching Hooks to every React user, and would require more explanation of what the instance parameter is when teaching React the first time. Also, if the instance parameter isn’t an opaque object, it’d need to be documented and subject to React’s backwards-compatibility guarantees. This change would require every custom hook library to be updated. It also conflicts with the old-style context API for function components (although I’m not sure if that’s still supported).

@phryneas
Copy link

phryneas commented Jan 9, 2020

In addition to that, this would not make function components any more pure, and assuming that function components should be pure is a fallacy.
There will always be hooks that hold external references, wait for external data, etc.
Oh, and then there's useEffect. Pure functions should not cause side effects.

The key to function components is idempotency, not purity.
Call it 1 time, or n times at a specific point in time -> always get the same result, side effects should only trigger once.
Call it at another point in time -> no guarantees, external dependencies might have changed.

So going for something that makes function components somehow seem more pure will just instill a wrong mindset in learners without any real benefit.

@0e39bf7b
Copy link
Author

@j-f1 imho it's easier to understand what does new parameter do than to understand why I have an empty state when React calls component's function for the first time and non-empty state when React calls it again. Even the idea of this hidden context is too complicated and it can influence the implementation of an application and React framework itself.

@0e39bf7b
Copy link
Author

So going for something that makes function components somehow seem more pure will just instill a wrong mindset in learners without any real benefit.

@phryneas I think my point of view is not clear enough. The main idea is not to convert functional components with hooks to pure functions. I agree with you - it can be impossible in some situations. The main idea is writing the code which is clear not only for experienced developers, but for juniors too. Let's take a look at such an example: I want to write a function which will receive some user's attributes from the database and fill existing user object.

First example:

async function fillUser() {
  const data = await fetchUserData();
  user.name = data.name;
  user.email = data.email;
}

This function is not pure and it modifies some global variable user. It's difficult to understand where this variable is stored and how it appears in the code of the function. It's not because this function is not pure, it's because this function uses hidden global variable.

Second example:

async function fillUser(user) {
  const data = await fetchUserData();
  user.name = data.name;
  user.email = data.email;
}

This function is still not pure but it receives user variable which will be modified as an argument. It makes code more understandable. To write a test for this function I need to provide an object as an argument and that's all. I don't need to find and modify some global variables. I can run several tests in parallel and I'll be sure that nothing will go wrong because every test uses it's own user object.

Talking about hooks - the situation even more sophisticated: there is a function useState and for people who is not familiar with React sources there is no idea how it stores the data and how it associate the data with some particular virtual DOM element. In this situation the developer has two options: think about useState as a magic or spend a lot of time exploring React source code. It can be almost impossible for the beginners.

Talking about useEffect. I think this hook does not perform any side effects right now, it just plan to perform some effects in some moment in the future. Introducing instance parameter makes it more clear:

useEffect(() => {
  // ...
}, []);

It's difficult to understand when this code will be executed because there is no explicit connection with some object of virtual DOM or something like this.

useEffect(() => {
  // ...
}, [], instance);

It this case it's more clear that this effect associated with this particular instance and for different instances there will be different effects.

@j-f1
Copy link

j-f1 commented Jan 10, 2020

I feel like a blog post like this does a pretty good job of explaining how hooks work without needing to add a parameter to every Hook call.

@mAAdhaTTah
Copy link
Contributor

there is a function useState and for people who is not familiar with React sources there is no idea how it stores the data and how it associate the data with some particular virtual DOM element.

My experience teaching hooks to people suggests this is not nearly as difficult to understand as you might think. Most people recognize that the function call is tied to the component and don't need a full understanding of the internals to use them. Threading down the instance object just moves the question to "what is this object and where does it come from?", which doesn't really make things that much clearer. It also exposes internals to users in a way that is probably undesired.

This proposal also doesn't take into account the fact that function component often have a second parameter: legacy context is provided via that parameter if contextTypes is set as a static property, and ref is provided there if the functional component is a ref-forwarding component.

@0e39bf7b
Copy link
Author

@j-f1 sorry, but I think that if you need to read a long post to understand how Hooks can be implemented it means that something wrong with this conception.

@0e39bf7b
Copy link
Author

@mAAdhaTTah I think the whole idea of React components is to write classes and functions which will be called by React, so the answer to the question "what is this object and where does it come from?" is simple - it's an object provided by React when it calls component function. Moreover it makes the behavior of the function component more obvious because it doesn't hide the idea that function component manipulates virtual DOM instance.

Taking about legacy context and ref - it's a good question. I'll think about it. Thanks!

@j-f1
Copy link

j-f1 commented Jan 13, 2020

I think that if you need to read a long post to understand how Hooks can be implemented it means that something wrong with this conception.

I don’t think that’s true. Just because something is hard to implement doesn’t mean it’s hard to use and understand. Also, the blog post isn’t that long.

@mAAdhaTTah
Copy link
Contributor

the answer to the question "what is this object and where does it come from?" is simple - it's an object provided by React when it calls component function.

This is not significantly more clear than "React manages the component state internally," especially if the object doesn't have an API you use, because the introduction of this object raises a whole host of new questions about the object you're interacting with. This is made even worse by the transition path: if the object is optional, but you get yelled at for not using it, what is React even doing with it? What is its purpose? And if it is to track state on, why can't you assign state to it directly? What else does it do? Is supposed to be opaque or something we actually use?

So this proposal introduces a bunch of syntactic overhead (the addition of an argument to every component and threading into every hook is not trivial–think of not only all of the hooks in your codebase, but all of the hooks currently published on npm), introduces a host of new questions that aren't relevant to a developer's ability to program with hooks effectively, and exposes internals to the user. That's a lot of drawbacks, and unless the user gets some new functionality or benefit out of it, that's a lot to overcome.

Making it less "magical" isn't necessary unless the magic is what's actively preventing people from learning & using hooks effectively. That doesn't seem to be the case, based on my own experience teaching them.

@phryneas
Copy link

Also, having this object will encourage people to abuse it.

  • "Why not manually modify it? "
  • "Why not store it in a global and share it between two components that way?"
  • "Why not pass it to another component as props so that that component can share state that way?"

People will come up with dozens of such dangerous patterns. And there's a really good reason why none of these is possible today.

@0e39bf7b
Copy link
Author

Just because something is hard to implement doesn’t mean it’s hard to use and understand.

@j-f1 unfortunately I can't agree with you. The devil is in the detail. Someone have to implement such sophisticated mechanism as Hooks and this code will be written by human and human can make mistakes which will be hard to find and debug because the implementation is tricky. And of course if you need to learn some new thing (a function which returns different values for the same arguments is a new thing especially for newcomers who never saw such behavior) you will spend more time than if you don't need to learn anything new.

@0e39bf7b
Copy link
Author

0e39bf7b commented Jan 20, 2020

@mAAdhaTTah could you please tell me is this implementation of sum function is OK with you:

let a;
let b;

function sum() {
  return a + b;
}

a = 10;
b = 20;

console.log(sum());

@mAAdhaTTah
Copy link
Contributor

@0e39bf7b Yes, cuz a React component that manipulates the DOM, manages its own state, & can perform side effects like making API calls is exactly equivalent to a sum function that uses free variables.

@0e39bf7b
Copy link
Author

@mAAdhaTTah I'm really appreciate you point of view but don't you think that in this case it's better to use function which receives data as arguments or write a class like

class Adder {
  constructor(a, b) {
    this.a = a;
    this.b = b;
  }

  sum() {
    return this.a + this.b;
  }
}

In this case it will be more obvious where the state is stored.

@mAAdhaTTah
Copy link
Contributor

@0e39bf7b It doesn't matter where the state is stored if the user never interacts with it directly. In both current hooks and your proposal, the user interacts with state through useState; they never interact with the data structure backing that hook.

@gaearon
Copy link
Member

gaearon commented Feb 10, 2020

Hi there! Thanks for the proposal.

We don't find these arguments compelling for the reasons already described in a comment on the original Hooks RFC: #68 (comment). You can look at Injection Model and Testing sections for more details. I believe all of these points are addressed there.

It also makes React code more robust especially for concurrent rendering because there are no global variables that can be corrupted by another Fiber (see React Fiber Architecture for more details).

React manages calling Fibers, so this is not a real issue.

@gaearon gaearon closed this Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants