Is the rewind approach safe?

[amannn]

I'm wondering how the rewind feature works internally. As far as I understand it simply outputs and removes the last rendered data that can be rendered on the server into a head. So is there something like a queue inside that the head data is being pushed on?

This assumes that the node.js server calls rewind as soon as possible after the render, right? Couldn't it happen that a new request comes in after a render that kicks off a new render and the rewind for the first request returns the data for the second request?

I might be totally wrong though – thanks in advance for the explanation! Very useful library btw.!

[potench]

It is up to the implementer to call rewind() which returns the current Helmet state. You should call rewind() directly after renderToString() to get the state of Helmet at the time of static HTML rendering.

Unclear what you are asking about "new requests" or "safe". "New Requests" on server create a totally new server connection and result in a new renderToString() and new Helmet rewind() state. "Requests" after renderToString() are reflected on the client-side only.

Please use Issues to report problems; your questions might be more appropriate for StackOverflow: http://stackoverflow.com/search?q=react-helmet

[amannn]

Thanks for the fast reply!

I did a bit more research and I found some related issues (seems like my concern is related to the underlying API of react-side-effect):

• #23 of react-side-effect
• #7 of react-document-title: There's also another description of my problem.

The important takeaway for me is that there should only be synchronous logic between renderToString and the call to rewind(), as async logic will introduce the race condition.

There are however also data fetching libraries that are rendering continuously in order to recursively fetch data for all components in the tree that need data (e.g. react-apollo does it – which is what I'm using – and also react-resolver I think). As far as I understand the node.js event loop, for these scenarios the data in Helmet can be inconsistent as the renders that are part of the data fetching can run in parallel – or rather parts of them after each other. Thus it's important that there's a final renderToString call after all the data fetching so the data for react-helmet will get into a correct state and can be pulled out.

I explained my findings in a bit more detail as maybe they could be helpful to someone else who is wondering about the same problem ;-).

[catamphetamine]

The short answer is: no, it is inherently thread-unsafe.
So it will break in case of using any of the alternative streamed React renderers.
http://formidable.com/blog/2017/introducing-rapscallion/

[goatslacker]

@potench can we reopen this issue?

Async rendering is going to become more and more popular in the very near future and this library takes advantage that React rendering is done synchronously at the moment in order to implement the .rewind() feature.

I don't think we need a solution just yet but it's definitely worth starting to think about how a library like this would work in an async world.

[catamphetamine]

For anyone interested, I refactored my framework today replacing react-helmet with my own @meta() decorator for setting <title/> and <meta/> tags:
https://github.com/catamphetamine/react-isomorphic-render/blob/master/source/meta.js

The approach used is:

• only <Route/>s can define <title/> and <meta/> tags
• they can do it only in this decorator
• the decorator can take data from Redux state for the tags
• when react-router matching finishes it yields a chain of components which are then searched for @meta(), the found @meta() is merged and the result is diffed with the current contents of <head/> to then update changed tags, remove no longer existent tags and add new tags

Usage:
https://github.com/catamphetamine/webpack-react-redux-isomorphic-render-example/blob/93dda10fcdc7887231c42952797143bdac3b52d2/client/src/pages/Users.js#L18-L23

It works in asynchronous environment because doesn't use react-side-effect (which react-helmet does) and therefore doesn't rely on global variables.
Therefore it's React 16 ready and works with streamed server-side rendering.

[tizmagik]

If React 16 is an option here’s a small lib that does head tag management in a thread-safe manner using React Portals: react-head 👍

[catamphetamine]

@tizmagik I looked at your library description and seems like it's no different from react-helmet because it only works with renderToString() which is not asynchronous.

[tizmagik]

Thanks for taking a look @catamphetamine -- the example app shows usage with renderToString(), yes, but since the HeadCollector component used for server-rendering takes in an array created in a per-request context, each array is unique to each request and should therefore support async rendering when it lands in a later version of React 16

[catamphetamine]

each array is unique to each request

In react-helmet it's the same: only a single request is being rendered at any given time because renderToString() is not asynchronous.

when it lands in a later version of React 16

It's already there

[tizmagik]

It's already there

My understanding is that it's not enabled/exposed in the latest release of React 16.0.0. If that's not the case, how do you enable it? Then I can run some tests and try it out!

[catamphetamine]

@tizmagik Server-side asynchronous rendering is already there while client-side asynchronous rendering will be enabled in future versions.
See https://hackernoon.com/whats-new-with-server-side-rendering-in-react-16-9b0d78585d67

[tizmagik]

@catamphetamine my understanding is that no async rendering features have been enabled in React 16 yet:

We think async rendering is a big deal, and represents the future of React. To make migration to v16.0 as smooth as possible, we’re not enabling any async features yet, but we’re excited to start rolling them out in the coming months. Stay tuned!

https://reactjs.org/blog/2017/09/26/react-v16.0.html#new-core-architecture

[catamphetamine]

@tizmagik They are enabled on the server side

[tizmagik]

Ok, well, if that's true then it seems to be working 😁

[catamphetamine]

I mean, asynchronous rendering is working but your library isn't: it's still synchronous on the server without the benefits of being asynchronous.

[tizmagik]

PRs are open 😁

[catamphetamine]

@tizmagik No PR can fix that, so it would have to be a completely different library.

[goatslacker]

@tizmagik @catamphetamine

You can use ReactDOMServer.renderToNodeStream in order to test out your library with streaming.

The problem with these libraries and streaming is that when you're streaming the response the <head> comes first. The allure of these libraries is that you can modify the <head> from deep within your React tree. In a streaming world, the component that modifies <head> might not be reached in time before we start streaming so it's impossible to maintain the same API and have streaming.

The best thing I can think of is you either pause streaming until you've received all the relevant header information. Or you switch to defining header overrides at the top-level, ideally tied to a route.

[catamphetamine]

You can use ReactDOMServer.renderToNodeStream in order to test out your library with streaming.

There's no need for tests because I can already tell you that it won't work

In a streaming world, the component that modifies might not be reached in time before we start streaming so it's impossible to maintain the same API and have streaming.

Not "might not", just "won't"

The best thing I can think of is you either pause streaming until you've received all the relevant header information.

The "relevant header information" could be anywhere by design.
Therefore there's no "until", there's only "the end".

Or you switch to defining header overrides at the top-level, ideally tied to a route.

I'm doing that in my own library

[tizmagik]

Thanks for the explanation @goatslacker -- yea it seems that streaming is fundamentally incompatible with being able to affect <head /> tags from deep within the component hierarchy. However, from what I can gather, the end result will be that the <head /> tags will be eventually consistent on the client because the client-rendering will update the head block.

Note that stream rendering and asynchronous rendering are not necessarily the same things! It will be interesting to see what React ends up doing as they roll out async rendering features in later version of React 16. 😁

@catamphetamine I think your use case is different. The whole point behind react-head and react-helmet is that you do not know statically (at the route level) what the <head> tags should be.

[catamphetamine]

I think your use case is different.

My use case is like everybody else's

the <head /> tags will be eventually consistent on the client because the client-rendering will update the head block.

Edit: no, that won't work and it won't be "eventually consistent"

Note that stream rendering and asynchronous rendering are not necessarily the same things

They are, in terms of this discussion

It will be interesting to see what React ends up doing as they roll out async rendering features in later version of React 16.

Asynchronous rendering features are already available on the server right now

[catamphetamine]

the tags will be eventually consistent on the client because the client-rendering will update the head block.

That's another story and that might be even a viable approach.
I'd say it is.

Oh, actually, no, it is not.
And it won't work.
There's simply no way for it to work because <head/> can't come after <body> has started and therefore it can't ever become "eventually consistent" because there's no "eventuality" in <head/> — it's a one-time thing.

[goatslacker]

@catamphetamine

Therefore there's no "until", there's only "the end".

There's a middle-ground that could be had here where we're able to modify the header info within children in a React tree but not necessarily at "the end". In that case, pausing the stream until that point is reached is doable. I've implemented a prototype of this approach already and it's 👍

[catamphetamine]

@goatslacker It's unclear how do you define "that point" because as react-helmet goes <meta/> tags can be inserted anywhere including the footer of the page so I see no "middle ground" in any case.

[tizmagik]

There's simply no way for it to work because can't come after has started and therefore it can't ever become "eventually consistent" because there's no "eventuality" in — it's a one-time thing.

I disagree 😁

[catamphetamine]

I disagree

Can't argue with that

[kouhin]

I forked this repository and create react-safety-helmet to resolve this problem.

Everyone can simply migrate codes.

import { createHelmetStore, HelmetProvider, Helmet } from 'react-safety-helmet';

// helmetStore must be created per request.
const helmetStore = createHelmetStore();
ReactDOMServer.renderToString(
    <HelmetProvider store={helmetStore}>
        <Helmet>
            <title>Fancy title</title>
        </Helmet>
    </HelmetProvider>,
    container
);
const head = helmetStore.rewind();

Also, React 16 renderToNodeStream is supported.

import stream from 'stream';

class DrainWritable extends stream.Writable {
    constructor(options) {
        super(options);
        this.buffer = '';
    }

    _write(chunk, encoding, cb) {
        this.buffer += chunk;
        cb();
    }
}

new Promise((resolve, reject) => {
    const writable = new DrainWritable();
    const helmetStore = createHelmetStore();
    ReactDOMServer.renderToNodeStream(
        <HelmetProvider store={helmetStore}>
          <App />
        </HelmetProvider>,
    ).pipe(writable);

    writable.on('finish', () => {
        resolve({
            body: writable.buffer,
            head: helmetStore.rewind(),
        });
    });
    writable.on('error', reject);
}).then(({body, head}) => {
    // Create html with body and head object
});

If possible, I will create a PR to react-helmet, but it will break the compatibility.

[catamphetamine]

@kouhin Looks ok, but I'm still not sure why do people want to not use streamed rendering.
Is there any real-world case when Title or Meta must reside deep down the component tree and not just at the top level of the page.

[tmbtech]

I'm going to reopen this issue for further investigation. Does anyone have a public repo I can fork and recreate the issue?

[goatslacker]

@tmbtech here's some code to get you started

const React = require('react');
const { Helmet } = require('react-helmet');
const { renderToNodeStream } = require('react-dom/server');

function Application() {
  return React.createElement('div', null,
    React.createElement(Helmet, null,
      React.createElement('meta', { charSet: 'utf-8' }),
      React.createElement('title', null, 'My Title'),
      React.createElement('link', { rel: 'canonical', href: 'http://mysite.com/example' })
    )
  );
}

const element = React.createElement(Application);

renderToNodeStream(element).pipe(process.stdout);

The problem here is figuring out where to put Helmet.renderStatic() and where you build up the document html, head, etc...for streaming you'll need the pre-content and post-content stuff before the content starts rendering. Good luck!

FWIW I ended up pulling title, meta, and friends off of the route config (which is static in our case) so we can resolve all of this before we start React rendering. It's still defined using React on the route config so you get the benefits of configurable meta per route without needing side effects.

[richardscarrott]

Wow, I've been using this library for years on many projects and had no idea it had such a critical bug!

Our crawler started to see information from protected routes 🤦🏻‍♂️ (fortunately nothing critical to date but the door is wide open here!)

We're possibly more exposed because in addition to Apollo's getDataFromTree we use renderToStringAsync.

Surely @kouhin's solution needs to be implemented here; same approach used by StaticRouter in react router exposing per-request state via a provider?

Or at the very least there should be a HUGE warning in the documentation explaining the risks?

[richardscarrott]

In case somebody else is interested, we ended up using https://github.com/staylor/react-helmet-async which solves the issue.

[richardscarrott]

I've created a PR #614 to add a warning