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

Async rendering and Aphrodite #245

Open
goatslacker opened this Issue Jun 9, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@goatslacker
Copy link

goatslacker commented Jun 9, 2017

Aphrodite currently relies on the fact that rendering with React is synchronous in order to build up/pull out the css definitions. The injectionBuffer is actually a module level "global" that just gets appended to: https://github.com/Khan/aphrodite/blob/master/src/inject.js#L140

While this works great at the moment, asynchronous rendering of React components are going to become popular in the near future and libraries like this will need to adapt.

Putting this issue on your radar so we can discuss how we'd go about safely living in an async world.

There already exists a few implementations that either stream, or render to a promise:

https://github.com/aickin/react-dom-stream
https://github.com/FormidableLabs/rapscallion

@xymostech

This comment has been minimized.

Copy link
Contributor

xymostech commented Jun 9, 2017

This has already been discussed a little bit in #63. In order to render styles that the page needs in the <head>, we need to know all of the css() calls that are made before you start sending down the rest of the page. That's inherently not compatible with async rendering, not because of our module-level global but because of the timing of when that rendering happens.

Random thought, though: We might be able to get away with rendering <style> tags inline before each block of HTML as each block is streamed? this SO answer mentions that <style> tags should go in the <head>, but most browsers support them being in the <body> too. Interesting. That approach is definitely not compatible with the library currently, but that's the only approach that might be reasonable that I can think of.

@goatslacker

This comment has been minimized.

Copy link

goatslacker commented Jun 9, 2017

we need to know all of the css() calls that are made before you start sending down the rest of the page. That's inherently not compatible with async rendering

Yes, adding support for streaming will definitely be a challenge. Async rendering isn't just streaming though, rapscallion supports rendering to a promise but we'd need Aphrodite to support multiple injection buffers on the server in that case so that they don't clobber each other.

@lewisf

This comment has been minimized.

Copy link

lewisf commented Sep 29, 2017

This might be tangentially related:

We call the render functions on our components during our data query collection phase (react-apollo) to walk the UI tree asynchronously. Without async support in this library it's impossible to turn off the injection which leads to "Cannot automatically buffer without a document" while this is happening.

We've currently forked Aphrodite so we could use continuation-local-storage to toggle this behavior on and off, but that's far from ideal :(

@lewisf

This comment has been minimized.

Copy link

lewisf commented Feb 20, 2018

Can we start a conversation around what it's going to take to make streaming support a reality? It looks like we're coming closer and closer to a reality where people building React applications are going to want it.

As @goatslacker mentioned:

Async rendering isn't just streaming though, rapscallion supports rendering to a promise but we'd need Aphrodite to support multiple injection buffers on the server in that case so that they don't clobber each other.

Is multiple injection buffers something that the maintainers of Aphrodite actually want to support with this project in the future? If so, it would be very helpful to have some guidance around what this should look like.

@kiznit

This comment has been minimized.

Copy link

kiznit commented Dec 23, 2018

I find myself in need of async rendering support as well. I am working with an asynchronous router (universal-router) that basically resolve routes to components asynchronously. The action() function can be asynchronous and this is why the resolve() function is as well. This can be used to fetch data for example before rendering components.

Example:

import { StyleSheet, css } from `aphrodite`;

const styles = StyleSheet.create({
    blue: {
        color: `blue`,
    },
});

const routes = {
    path: `/`,
    action: () => (
        <h1 className={css(styles.blue)}>
            Hello, world
        </h1>
    ),
};

Here is the rendering function (it resolves the route and then wrap it up in an component that know show to inject scripts, css, and so on:

const render = async (req, res) => {
    const content = await router.resolve({
        pathname: req.path,
    });

    const components = (
        <Html css={...}>
            { content }
        </Html>
    );

    return renderToStaticMarkup(components);
};

The problem I am facing is this: I can't call StyleSheetServer.renderStatic() after the route is resolved (it is too late as my route's action() gets called during router.resolve(). I get the expected "Error: Cannot automatically buffer without a document".

Passing in my whole render function to Aphrodite doesn't work either because it is asynchronous:
StyleSheetServer.renderStatic(async () => await render(req, res));

Now if I understand this thread and the code correctly, simply updating renderStatic() to handle async functions wouldn't be enough as there is only one buffer.

@bkonkle

This comment has been minimized.

Copy link

bkonkle commented Jan 19, 2019

I'm hitting this issue as well. When two requests come in at the same time, they step on each other. As long as the requests are simultaneous, I'm consistently getting Cannot buffer while already buffering.

I'm using reset, startBuffering, and flushToString independently. The above happens if I call reset() after flushToString(). If I call reset() before startBuffering() instead, I get "Cannot automatically buffer without a document". If I do individual requests rather than simultaneous requests - all is fine.

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