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

Support Asynchronous Components #526

Closed
neeharv opened this issue Feb 2, 2017 · 18 comments
Closed

Support Asynchronous Components #526

neeharv opened this issue Feb 2, 2017 · 18 comments

Comments

@neeharv
Copy link

neeharv commented Feb 2, 2017

Inspired by https://twitter.com/youyuxi/status/824659082816716802

It'd be great if we could do the same with Preact. Looking forward to a discussion about what would it take to add support. Off the top of my head, this would possibly need

  1. Async rendering support
  2. Caching of ^ so subsequent renders are immediately resolved
  3. Update h to support Promises as children

A common argument against this would be to have this in userland code, however there are legitimate cases where having this baked into the framework itself would strongly encourage developers to start using code-splitting, which I think fits into Preact's goals of performance and min size.

@developit
Copy link
Member

Love the idea, but I can't think of a way to implement this that would work in Preact's core.

@thysultan
Copy link

thysultan commented Feb 3, 2017

@developit, depending on how you implement forceUpdate, if import returns a promise... you could just replace the render method with one that returns the cached response while rendering an empty element.

.then((value) => {
    component.constructor.render = component.render = () => {return value} ;
    component.forceUpdate();
})

return h('noscript');

and end with calling forceUpdate once the promise has resolved, forceUpdate will now call a render method that returns the resolved and cached response, when it compares this with what is currently rendered(an empty noscript element) it replaces it.

@developit
Copy link
Member

That's quite similar to how the userland solutions work actually (implemented this today!)

@alexeyraspopov
Copy link

I'm glad I've found this issue.

I can't think of a way to implement this that would work in Preact's core.

Since import() is just an async function, this problem is just about supporting functions that return promises. By just implementing this opportunity we can cover bigger range of problems, such as co-located data fetching.

Some time ago I wanted to solve the data fetching problem with minimum impact to the project and as less API as possible. I found out that the easiest and idiomatic way is to support async function as a component.

async function ParentComponent(props) {
  const data = await SomeAPI.retrieve();
  return <ChildComponent data={data} />;
}

I use React in my current project and I came up with the next solution: react-coroutine. It supports async functions and async generators.

Worth mentioning, right after I've started using async components I found that a lot of things can be solved just by using language features, no libs or APIs.

async function ParentComponent(props) {
  try {
    const data = await SomeAPI.retrieve();
    return <ChildComponent data={data} />;
  } catch (error) {
    return <details>
      <summary>An error occurred while fetching data</summary>
      <p>{error.message}</p>
    </details>;
  }
}

After Webpack 2 released, I figured that react-coroutine supports import() without any changes, just because of async nature. You can find an example of using import() within async component here:

examples/code-splitting/modules/Routes.js#L18-L25

And example of using async generator with import() here:

examples/code-splitting/modules/Routes.js#L27-L45

Using async generator in particular you can easily combine code splitting with loading spinner and co-located data fetching. It does not require any separate libs for different purposes and APIs, it just works because of async nature.

I wish React had async components supported by default and I'm not sure if the team can support this suggestion (since there are already implementations in the user land). Though, I think it is important to have this support since it makes easier for developers to start using code splitting, co-located data fetching and a lot of other possible use cases of async functions/generators.

@developit, I want to help to implement this in Preact but I need some help/introduction to the codebase.

@developit
Copy link
Member

developit commented Mar 16, 2017

The handling would go here, but honestly the whole chain to that point is synchronous without #409 being merged yet, so I'm not sure how that could be made to work. Preact also can't rely directly on Promises in core since it has to support IE9.

@Satyam
Copy link

Satyam commented Mar 29, 2017

I would like to suggest to add the ability for componentWillMount and componentWillReceiveProps to return a Promise, in which case the rendering would be delayed until the Promise is resolved. I meant to offer a working example and I modified setComponentProps and tried to make some examples to show it working but, unfortunately, due to my lack of deep knowledge of Preact, failed to make it work.

The Readme file on the example code explains what I tried to do and how to use it.

The existing tests on the modified Preact run fine, so the change is backward compatible.

I am sorry I am new to Preact, I hoped I could be more helpful.

Thanks

@Satyam
Copy link

Satyam commented Mar 30, 2017

I am guessing here but I imagine that what I need to do is pile up all the promises returned from componentWillMount and/or componentWillReceiveProps and then make the diffing of the new vdom against the existing run on a Promise.all() of all these pending promises. If the array is empty, the diff would run right away, otherwise, it would wait until all the renders, either synchronous or queued, are done. I am still trying to find my way over this code, any ideas as to where to look? Or whether I'm on the right track? Thanks again.

@developit
Copy link
Member

developit commented Mar 30, 2017

@Satyam It would work the same way as #409 really. That said, I'm not sure I follow what async componentWillMount() would give us - by that point we've already instantiated the component, so the cost has been paid (same deal for componentWillReceiveProps()). Paul's PR introduced the idea that we could defer even instantiating a component until there were free cycles to do so, which I think is the only effective way to defer work (unless I'm missing something?).

@Satyam
Copy link

Satyam commented Mar 30, 2017

@developit If I am not reading it wrong, the idea of #409 is that, if new data arrives when the components are rendering, interrupt them and start with the new data. What I mean to do is that instead is to hold on until the data is there. Specially in SSR, you only have one chance with res.send() so when it happens, it better be good.
In both the methods I suggest changing, this.props is already populated and components can go and fetch the data they really need. And they can render the children they actually need, like the todo-item components in a todo-list, if you don't wait for the data, how could you know which children to render? Doing it in the traditional way it requires the developer to somehow device ways to fetch the data based on the URL so that everything is there before attempting to call renderToString. If you do it when mounting then you are using the same logic (routing, conditional rendering, etc) that your application will use. Since data fetching can take a while, it serves no purpose to optimize a make-do rendering that will have to be discarded. If the waiting time is mostly in fetching the data, optimizing the render makes no difference, better to wait and do it right. I want to render based on actual data, not on the availability of free cycles.

@developit
Copy link
Member

The key with Paul's PR is as you described: components are progressively instantiated as CPU is available - they won't even be invoked/rendered/anything until the scheduler kicks in. This is useful because it means if your data fetching beats the progressive rendering, your first render will be with the fetched data, so nothing is wasted. If CPU time is available before data comes in, components will be "booted" and can show a UI indicating loading.

More generally though, it seems a little bit odd to fetch data on the server and do a synchronous static render, but then also re-fetch that same data on the client to do an asynchronous render that likely produces an identical DOM tree. Most people who do full SSR as you describe also send down the data to be used for hydration, so that components boot into a cached state instead of a loading state. Maybe that would be a simpler approach?

The problem with making any of the lifecycle methods truly asynchronous (eg: promise-returning) is that it introduces a very real locking issue - if a (parent) re-render is triggered while a component is in an unbooted state, does it boot? If the DOM is mutated outside the control of Preact before a component has booted - is that left in-place somehow (impossible to know that it happened), or is the now mismatched DOM mutated to match the initial render of the component?

These are a couple of the main reasons VDOM libs don't tend to rely on asynchronous lifecycle methods, rather choosing to implement asynchronicity internally within the diffing algorithm. When internally implemented, it can be halted or cancelled.

That said, #409 has been a long time coming because I'm trying to integrate a pluggable scheduler into it. That would allow you to write a simple scheduler that asks components when they would like to boot, which I think is a very elegant solution to the issue you laid out. You'd define a static method on your components (something like next.js' getInitialProps()) that is given control over the initial boot of the component. Until that method's returned Promise resolves, the component is left unbooted and any SSR'd DOM left in-place.

@scurker
Copy link
Contributor

scurker commented Mar 30, 2017

More generally though, it seems a little bit odd to fetch data on the server and do a synchronous static render, but then also re-fetch that same data on the client to do an asynchronous render that likely produces an identical DOM tree.

It might seem a little odd to want to do this, but there are some very practical user benefits from doing so. Given that javascript can be blocking for a number of different reasons (network, parsing, etc.) by providing an initial SSR state that is already valid, you can ensure that the user already has content ready to go regardless of the current state of javascript. In a more real life example, we had an issue with our CDN recently where assets were taking upwards of 5 second to download over a fast wifi connection. Had we not had valid async SSR content delivered, it would have had an adverse impact on our users. It didn't matter that the content eventually got rehydrated with an initial state because it was already valid.

@developit
Copy link
Member

Isn't that just display content though, with no interactivity?
FWIW I didn't mean to dismiss the approach, just that is one reason I've avoided it. I also don't do much SSR either so I'm in a bit of a minority, haha (I just do prerendering and deploy everything to CDNs)

@scurker
Copy link
Contributor

scurker commented Mar 31, 2017

Correct. It's a bit of uncanny valley since the time to interactive is still delayed until after scripts, but some content is better than no/placeholder content until after blocking javascript.

It's an approach that has unfortunately been left to userland, so there's a lot of haphazard magic to sometimes make it work correctly. And unfortunately I think some people tend to avoid it because of the potential difficulty. Maybe Async SSR is a whole other issue, so I don't want to commandeer this issue, it's just been something on my mind.

It's not necessarily something that would need to go into Preact's core, but rather something like preact-async-render-to-string.

@developit
Copy link
Member

You might find this thread interesting - it's a super simple full async SSR:

preactjs/preact-render-to-string#30 (comment)

@Satyam
Copy link

Satyam commented Mar 31, 2017

@developit

it seems a little bit odd to fetch data on the server and do a synchronous static render, but then also re-fetch that same data on the client to do an asynchronous render that likely produces an identical DOM tree

It is not my intention to do that. As I mention towards the end of this section, a Redux store could be populated and it serialized contents sent to the client, where it can be re-hidrated. Though a developer my blindly query the source for data when invoked, it would be sensible to check whether the data is already there and, if it is already available (from the SSR rendering or whatever) the Promise would be resolved immediately. In the end, whether the developer does this or not is no argument against the feature being available. The examples shown in my sample code are oversimplified, I normally use Redux and hardly ever fetch data blindly.

I defer to you regarding the locking and refreshing issues, if that is a show-stopper, then there is no further discussion.

I don't mind if existing methods are repurposed as I suggest or a new getInitialProps method is defined. I don't agree on it being static as you suggest as the purpose of it is to do whatever it does based on this.props and possibly this.state. Of course a static method could receive props and state as arguments, as render does, but then it departs from the standard of other lifecycle methods. I don't mind adding props and state as sugar, but making that the only means of accessing them promotes the already too frequent overuse of this.state for storing all information which might not always require a refresh and taxes the application with pointless rerenders due to changes to state.

However, I do prefer to have componentWillReceiveProps changed to return Promises because by having access to this.props and also nextProps as an argument, it helps in deciding whether a data fetch is really necessary.

@Satyam
Copy link

Satyam commented Mar 31, 2017

Actually, I think I have a sort of middle ground here. When using renderToString the rendering process effectively pauses while the data is fetched and it does get into the HTML. My problem there is that somehow, this line doesn't render the imported stateless component but the result of toString. Anyway, this is besides the point.

When rendering on the client side, it would be acceptable for the render not to be paused. However, it is important that the components get re-rendered with the newly received data. This is no problem when using Redux, as any change in the data store would trigger such a re-render, but there is no Redux in this example to do it. Moreover, it would be important that the re-render be queued once all the requested data arrives, to avoid pointless intermediate states. Stateful components might chain a forceUpdate at the end of the Promise, but that would be inefficient as each Promise resolved would trigger a new re-render.

So, my suggested behaviour is:

  • with renderToString: pause until Promise resolved (it already does so)
  • with render: keep going but queue a re-render of all the components involved when all the Promises get resolved.

@robertknight
Copy link
Member

Given that React now has an official API for loading components lazily (Suspense), perhaps we should close this issue in favour of one for implementing an API compatible with React.lazy?

@developit
Copy link
Member

Agreed @robertknight. We have support for Suspense-based deferred hydration and tree creation landing in #2291 / #2214 and that will likely be the most logical path forward here. It avoids the pitfalls mentioned in this thread relating to deadlocked async trees, and doesn't rely on the renderer dealing with concurrent asynchronous trees.

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

7 participants