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 type to root, remove OpaqueRoot. #28416

Closed
wants to merge 5 commits into from

Conversation

bubucuo
Copy link
Contributor

@bubucuo bubucuo commented Feb 22, 2024

I recently read the React source code, and have a few suggestions to improve the reading experience. Hope you can consider them ~

  1. Every time I see the root assignment in the createRoot function, I often can't remember the type of this value for a moment, so maybe adding a type here would be helpful.
  2. OpaqueRoot is actually FiberRoot, so there's no need to define another name for it here, right? It's not very good for the reading experience~

I recently read the React source code, and have a few suggestions to improve the reading experience. Hope you can consider them ~
1. Every time I see the assignment of root in the createRoot function, I often can't remember the type of this value for a moment, so I think maybe adding a type here would be helpful.
2. OpaqueRoot is actually FiberRoot, so there's no need to define another name for it here, right? It's not very good for the reading experience~
As before, add type annotations to the root.
As before, add type annotations.
As before, add annotations.
As before, add annotations.
@sebmarkbage
Copy link
Collaborator

There's a point to the separate name. It's also supposed to be an opaque type ideally.

It's because the outside of this shouldn't know whether it's a FiberRoot or not. It is today but after an internal refactor of the implementation details it wouldn't be.

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.

None yet

3 participants