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

First pass context is lost in Call - Return #12638

Closed
rmhartog opened this issue Apr 18, 2018 · 4 comments
Closed

First pass context is lost in Call - Return #12638

rmhartog opened this issue Apr 18, 2018 · 4 comments

Comments

@rmhartog
Copy link
Contributor

rmhartog commented Apr 18, 2018

I have been really enjoying my explorations into building better 'compound components' using the experimental react-call-return. This issue is intended to start a discussion of some of the shortcomings of the current API that I ran into.

Experiments in which the return is used as a leaf node that yields some data are very successful. Since #11955 was solved I have not run into further issues with the stability of this feature.

However, for 'compound component' usage, to implement features such as layout, my return often yields an element or render prop. In these cases, these elements are rendered with the call as the parent, and any context created in the first pass (between the call and return) is lost.

A reproduction case can be found here: https://codesandbox.io/s/0p4lvy72pl, as an end-user of these components (unaware that they use call-return internally), I would expect to see 'Greetings 1' instead of 'Default 1'.

I don't consider this a bug, but rather a side-effect of how I'm using the API. However, I believe the use-case above is a valid one and providing an API that supports this would be beneficial to library authors. Below I'll share some thoughts on such an API for discussion.


Initially, the API was called coroutine and yield, which suggested a subtree would yield and later resume. Presumably, this is why the API was renamed. My suggestion would be to implement the coroutine-yield functionality, which continues rendering the 'continuation' as children of the yield fiber. Of course I am not aware of all the choices that led to the current API, so input here is welcomed.

createCoroutine(children, handler, props) would function very similarly to the current createCall, but the handler does not return the children to render. Instead, it returns some aggregated value. This value is passed to the second argument of createYield(value, continuation, props), together with that yield element's props and index within the coroutine. The element returned from the continuation is reconciled with the yield fiber's children, preserving its position in the tree and thus also any context that was accumulated between the coroutine and yield.

Note that the new API would be a strict superset of the current implementation, I imagine the call-return could be written with coroutine and yield as follows:

const createCall = (children, handler, props) => createCoroutine(
  [
    createYield(null, (props, values, index) => handler(values.props, values.yields), props),
    ...React.Children.toArray(children),
  ],
  (props, [_, ...yields]) => ({ props, yields }),
  props
);

const createReturn = value => createYield(value, () => null);

I've dived into the reconciler implementation for call and return, and implementing an API like the above seems feasible to me. I'd definitely be willing to give it a shot once the approach is clear. However, I am not aware of all the decisions that led up to the current implementation, and the exact impact on performance (increased tree traversals) etc...

CC'ing @sebmarkbage and @gaearon

EDIT: I've prototyped this API on CodeSandbox here: https://codesandbox.io/s/480nx1qw97, it causes multiple renders using setState and forceUpdate, and uses some nasty traversals of _reactInternalFiber, it is highly unstable, but it illustrates the idea outlined above.

@rmhartog
Copy link
Contributor Author

rmhartog commented May 4, 2018

Since there hasn't been any discussion, I am unsure how to move forward with this. I am not confident moving forward with creating an RFC, since the approach is still unclear.

I realize that the initial post is quite long and perhaps slightly vague, any input or clarifying questions are welcomed.

@aweary
Copy link
Contributor

aweary commented May 19, 2018

The call/return package was deleted in #12820, so this no longer applies. Thanks for the detailed report though!

@aweary aweary closed this as completed May 19, 2018
@gaearon
Copy link
Collaborator

gaearon commented May 19, 2018

We’ll likely want your feedback though when we start working on a replacement for these use cases. Appreciate the write up.

@rmhartog
Copy link
Contributor Author

Thanks for the heads-up, if there is a way for me to join the discussion, I would be happy to. I believe that an API in this area could really strengthen some of the abstractions we're trying to make. Layout is only one of those cases!

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

No branches or pull requests

3 participants