Khan / aphrodite

Framework-agnostic CSS-in-JS with support for server-side rendering, browser prefixing, and minimum CSS generation
5.34k stars 188 forks source link

Async rendering and Aphrodite #245

Open goatslacker opened 7 years ago

goatslacker commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

srolel commented 4 years ago

Hi all, I have a working solution for this and could make a PR, but are PRs even looked at?

ljharb commented 4 years ago

Always better to file it and have it ignored, then never file it just in case it might be ignored :-)

srolel commented 4 years ago

Done :P https://github.com/Khan/aphrodite/pull/390