The React.ReactNode type is a black hole

As developers, we use TypeScript for a few different reasons. The self-documentation aspects are huge – being able to step into an unfamiliar function and know the shape of the objects it's expecting is a massive boon when working on a large project. The added tooling features, with IntelliSense and its ilk, are also a big help for productivity. But to me, the most important reason to use a strongly typed system is to eliminate an entire class of runtime bugs, where a function gets passed an object it doesn't know how to handle and fails at runtime.

It's that last reason that leads to the purpose for this post. I recently handled a bug where a React component was throwing an exception at runtime. The source of the issue was a recent refactor done when internationalizing this area of our application, where a prop expecting a renderable React.ReactNode was accidentally getting passed an object of class TranslatedText which could not render.

This is exactly the sort of bug we would expect TypeScript to catch at compile time!

How did this happen? At a high level it is because the React.ReactNode type included in DefinitelyTyped, used in hundreds of thousands of codebases around the world, is so weakly defined as to be practically meaningless.

We discussed this at a high level during the TIL segment of JS Party #213, but I thought it deserved a more rigorous treatment.

Come along as I share the exploration, why this bug has lingered in the wild for more than 3 (!) years since it was originally reported, and how we worked around it in our codebase to get ourselves protected again.

The Situation

It started with a simple bug report:

When I click on "Boost nudges" and attempt to select a filter group, I get an error saying something went wrong. This feature is vital for a demo I have tomorrow.

My first check was to see if I could reproduce it in the production application. I could. Next was to fire up a developer environment so I could get a useful backtrace, and the error was extremely clear:

Interpretation: React was trying to render something that it could not render. Using the file and line numbers to track down more, I could see that the object in question was a prop called description with the following type definition:

description: string | React.ReactNode;

The caller was passing it instead a TranslatedText object, which is a class we use in our system to handle internationalization. The expected use is that this object is passed to a <T> component that knows how to use it and a library of strings to render text in the correct language for the current user.

Having seen this: The fix was super simple. Wrap the TranslatedText object in a <T> component before passing it in as a prop.

With this patch in place, the immediate bug was resolved, and the demo mentioned in the ticket unblocked.

Understanding how the bug came to be was super simple - this portion of the application had only recently been internationalized, and the bug was introduced in that work. But then the real puzzle started: Isn't this type of bug exactly what using TypeScript and types is supposed to prevent? How in the world had the type system allowed something that was not renderable by React to be passed into a prop with type string | React.ReactNode?

The trail

When I first saw that this problem wasn't being caught, my initial thought was maybe for some reason type checking wasn't getting run at all. Maybe we had a bug with cross-module calls, or there was a problem in our configuration. But I was quickly able to rule this out by but reducing the prop type to string and seeing that it triggered a type error.

The next thing I tried was testing to see if somehow TranslatedText was somehow implementing the React.ReactNode interface, but adding a quick implements  annotation to TranslatedText (i.e. class TranslatedText implements React.ReactNode) resulted in the compiler throwing an error. That matched my expectations, because it DOESN’T implement the interface - if it did, we wouldn't have had this problem in the first place!

I then started diving down into the way that React.ReactNode was defined. These definitions are coming from DefinitelyTyped, the canonical open source repository of type definitions for npm packages that don't natively include types, and the key definitions look like this:

    type ReactText = string | number;
    type ReactChild = ReactElement | ReactText;

    interface ReactNodeArray extends Array<ReactNode> {}
    type ReactFragment = {} | ReactNodeArray;
    type ReactNode = ReactChild | ReactFragment | ReactPortal | boolean | null | undefined;

There it is, in the ReactFragment definition!

The ReactFragment, which is included in the ReactNode type, includes an empty interface. Due to the way that TypeScript handles excess property checks, this means that the ReactNode type will accept any object except an object literal. For almost all intents and purposes, it is functionally equivalent to an any type. Even though most functions using this type will expect it to mean "something renderable by React".

At this point I brought this back to our team at Humu:

As folks dug in one of our team members discovered that that this has been a known issue since 2018! There is a discussion that implies an intent to fix the issue, but concerns about the ripple effects of introducing a fix, and no progress for the better part of a year.

First attempts at a fix

As we started looking at ways to address this issue in our codebase, we considered two options:

1. Moving everything in our codebase to a custom type
2. Using patch-package to update the React.ReactNode definition

Assessing the pros and cons of these different approaches, we felt that the patch-package approach would require fewer code changes and less ongoing cognitive load, but would have the disadvantage of requiring an additional dependency (and associated transient dependencies) and make it perhaps less visible what's going on.

In the end, we decided to try patch-package first because it would be less work. The change was super simple; we attempted a patch to the ReactFragment type that looked very much like the one that was proposed in the DefinitelyTyped discussion thread:

type Fragment = {
  key?: string | number | null;
  ref?: null;
  props?: {
    children?: ReactNode;
  };
}

While this approach didn't trigger any internal typing issues within our codebase, and resulted in the type system being able to catch the class of error that had bitten us at the beginning, it resulted in cascading type errors in calls into several React ecosystem libraries. We ran into troubles at the interface of our code into react-beautiful-dnd:

After diving down the rabbit hole and trying to figure out those type issues for a little while, only to have every change result in more and more type challenges, I decided that this would require someone with more TypeScript chops than me to figure out.

The Second Approach

The second approach we tried was to create a stricter type in our codebase, find/replace to use it everywhere, and then add a linter to keep it from being used. The types file we ended up with was very similar to the one we'd tried in the patch approach:

import { ReactChild, ReactPortal, ReactNodeArray } from 'react';

export type StrictReactFragment =
  | {
      key?: string | number | null;
      ref?: null;
      props?: {
        children?: StrictReactNode;
      };
    }
  | ReactNodeArray;
export type StrictReactNode =
  | ReactChild
  | StrictReactFragment
  | ReactPortal
  | boolean
  | null
  | undefined;

After verifying that this type actually caught the types of type error that we were trying to prevent, it was time to make the replacement across our codebase.

I briefly explored using jscodeshift to automatically make the replacement. I started going down that road, but I have no prior experience using jscodeshift and it was proving tricky. As I had limited time, I decided that our codebase was small enough that running find/replace in VS Code plus manually adding the import would be tractable and much faster than continuing to try to figure out jscodeshift.

NOTE: If anyone wants to write this codemod and send it me, I'd be happy to include it as an addendum to this post with a shoutout to you!

One PR later, we had a much safer codebase using StrictReactNode everywhere, but there was one step left to make this sustainable.

Writing an ESLint plugin

The reason React.ReactNode had permeated our codebase is that it is such a logical type to use in many situations. Any time you want to assert a prop is renderable by React, it's natural to reach for React.ReactNode.

Now we need all of our developers to instead reach for StrictReactNode. Leaving this to developer discretion or requiring this to be a part of manual code review and/or education seemed untenable, especially in a rapidly growing company like Humu.

To enforce the new practice and make it seamless to keep our codebase up to date and safe, we decided to write a custom ESLint linter to check for React.ReactNode and throw an error with a pointer to our preferred type.

This post is not about how ESLint plugins work, but in case you want to use it here is the plugin we arrived at:

module.exports = {
    create(context) {
        return {
            TSTypeReference(node) {
                if (
                    node.typeName.type === 'TSQualifiedName' &&
                    node.typeName.left.name === 'React' &&
                    node.typeName.right.name === 'ReactNode'
                ) {
                    context.report(
                        node,
                        node.loc,
                        'React.ReactNode considered unsafe. Use StrictReactNode from humu-components/src/util/strictReactNode instead.',
                    );
                }
            },
        };
    },
};

Now if someone does by accident try to use React.ReactNode in a type declaration, they get an error that looks like this:

Linting is a part of our CI testing that occurs before any branch can be merged, so this prevents anyone from accidentally pulling in the unsafe React.ReactNode type and points them to the replacement type instead.

Update: Mathieu TUDISCO wrote a more generalized eslint plugin with a fixer!

Wrapping Up

From my perspective, the entire goal of using TypeScript and a type system is to be able to prevent an entire class of bugs and make refactors like the original one that sparked this safe to do.

Having a wide open type like this in a super commonly used library is super scary. Time permitting, I will continue to work on getting this patched in DefinitelyTyped, but the ecosystem problem is large enough that this is unlikely to happen in a timely manner. Changes of this magnitude create a massive wave of ripples and types that need to be updated.

In the meantime, I highly recommend using an approach like our StrictReactNode to protect your codebase.