DefinitelyTyped icon indicating copy to clipboard operation
DefinitelyTyped copied to clipboard

feat(react): strict compile-time memoization checks

Open arthurgeron opened this issue 7 months ago β€’ 18 comments

Problem:

React performance can be significantly impacted by unnecessary re-renders. A common cause is using object or function references (created anew on each render) as props to components wrapped in React.memo or using them in hook dependency arrays (useEffect, useLayoutEffect, useCallback, useMemo, etc.). This defeats the purpose of memoization, leading to components re-rendering even when their inputs haven't meaningfully changed.

While static analysis tools like ESLint plugins (eslint-plugin-react-hooks, eslint-plugin-react-usememo) can help detect some of these patterns, they operate at lint time, have limitations based on AST analysis (often missing cross-file issues), can produce false positives/negatives, and only provide suggestions rather than guarantees.

Solution:

This pull request introduces compile-time guarantees for React memoization directly within the TypeScript type definitions for React. By leveraging TypeScript's type system, we can enforce that non-primitive props and dependencies are correctly memoized, catching potential performance issues as you type within your IDE.

Key Benefits:

  • βœ… Compile-Time Safety: Errors are caught during development, not at runtime or lint time.
  • πŸš€ Zero Runtime Overhead: This is purely a type-level enhancement; no changes to runtime behavior or bundle size.
  • πŸ’‘ Precision: Leverages TypeScript's full type-checking capabilities, including cross-file analysis, offering more accurate checks than AST-based linters.
  • ✨ Automatic Enforcement (Even for Libraries!): Because React's core types are augmented, these checks automatically apply to any standard hook – including those imported from third-party libraries – without extra configuration. Consumers immediately know if a prop or dependency requires memoization.
  • 🧐 Explicit Type Intent: Allows library authors or application developers to explicitly signal (and enforce) that components or custom hooks expect memoized values where necessary, improving API clarity.
  • 🧹 Reduces Linter Dependency: Largely eliminates the need for specific ESLint rules (eslint-plugin-react-usememo, etc.) aimed solely at checking prop/dependency memoization, as TypeScript now handles this enforcement robustly.
  • 🀝 Improved Developer Experience: Provides immediate feedback in the IDE, leading to faster debugging and more performant code by default.

Implementation Details:

  1. Memoized<T> Branded Type: A nominal type Memoized<T> (using T & { readonly __memoized: unknown }) is introduced to mark values explicitly returned by useMemo and useCallback.
    • Type Compatibility: Memoized<T> is assignable to T. However, T is only assignable to Memoized<T> if T is a Primitive. This ensures non-primitives must be explicitly memoized.
  2. Stricter Hook Dependencies: A MemoizedDependencyList type replaces the standard DependencyList for hooks (useEffect, useLayoutEffect, useCallback, useMemo, useImperativeHandle, useInsertionEffect). This list only allows primitives or Memoized<T> values.
  3. Hook Return Types: useMemo, useCallback, and the state value returned by useState are updated to return/be typed as Memoized<T>.

Memo is not covered for now, as the complexity of the types are pushing the limits of TypeScript resolution capabilities. History & Context:

The concept originated with lint-time tools like @arthurgeron/eslint-plugin-react-usememo. An attempt to implement this at the type level via declaration augmentation was made in @arthurgeron/react-memo-types but faced TypeScript declaration merging issues. This PR properly integrates the checks directly into the canonical React types.

BREAKING CHANGE:

Code relying on passing certain unmemoized non-primitive values will now fail type checking: - Non-primitive values used in hook dependency arrays (useEffect, useMemo, etc.) must be primitive, originate from hooks that return Memoized values (like useState, useMemo, useCallback).

Common patterns like using state directly from useState in a dependency array will not break due to this change, as useState's return value is now typed as memoized. However, values derived from state, refs, values from custom hooks not returning Memoized types, or functions/objects created directly within render scopes will trigger errors if used unmemoized in a dependency array.


Checklist:

  • [X] Use a meaningful title for the pull request. Include the name of the package modified. (feat(react): Add strict compile-time memoization checks)
  • [X] Test the change in your own code. (Compiled and linted successfully within the repo.)
  • [X] Add or edit tests to reflect the change. (test/hooks.tsx was updated.)
  • [X] Follow the advice from the readme.
  • [X] Avoid common mistakes.
  • [X] Run pnpm test react. (Ran pnpm -w run lint react which passed.)
  • [X] Provide a URL to documentation or source code which provides context for the suggested changes:
    • Initial ESLint plugin concept: https://github.com/arthurgeron/eslint-plugin-react-usememo
    • Previous external augmentation attempt: https://github.com/arthurgeron/react-memo-types
    • General React memoization docs: https://react.dev/reference/react/memo, https://react.dev/reference/react/useMemo, https://react.dev/reference/react/useCallback

arthurgeron avatar Apr 06 '25 17:04 arthurgeron

@arthurgeron Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped πŸ‘‹ β€” I'm the local bot who will help you through the process of getting things through.

This is a live comment that I will keep updated.

8 packages in this PR

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

  • βœ… No merge conflicts
  • βœ… Continuous integration tests have passed
  • πŸ• A DT maintainer needs to approve changes that affect more than one package

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 123 days β€” it is still unreviewed!


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 72416,
  "author": "arthurgeron",
  "headCommitOid": "fa868133199781eb3ac288cafbde472031d9e00b",
  "mergeBaseOid": "51aa14443b452199eb327421678d0a5ee8293c1c",
  "lastPushDate": "2025-04-06T17:40:56.000Z",
  "lastActivityDate": "2025-07-08T10:57:55.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "hippy__react",
      "kind": "edit",
      "files": [
        {
          "path": "types/hippy__react/hippy__react-tests.tsx",
          "kind": "test"
        }
      ],
      "owners": [
        "zerosrat"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "react-dragula",
      "kind": "edit",
      "files": [
        {
          "path": "types/react-dragula/react-dragula-tests.tsx",
          "kind": "test"
        }
      ],
      "owners": [
        "AdrianMrn"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "react-form",
      "kind": "edit",
      "files": [
        {
          "path": "types/react-form/react-form-tests.tsx",
          "kind": "test"
        }
      ],
      "owners": [
        "cameron-mcateer",
        "TiuSh",
        "Toliak"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "react-relay",
      "kind": "edit",
      "files": [
        {
          "path": "types/react-relay/test/relay-hooks-tests.tsx",
          "kind": "test"
        }
      ],
      "owners": [
        "captbaritone",
        "alloy",
        "maraisr",
        "edvinerikson",
        "levibuzolic"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    },
    {
      "name": "react-table",
      "kind": "edit",
      "files": [
        {
          "path": "types/react-table/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react-table/react-table-tests.tsx",
          "kind": "test"
        }
      ],
      "owners": [
        "ggascoigne",
        "gargroh",
        "riceboyler"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    },
    {
      "name": "react",
      "kind": "edit",
      "files": [
        {
          "path": "types/react/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/test/hooks.tsx",
          "kind": "test"
        }
      ],
      "owners": [
        "johnnyreilly",
        "bbenezech",
        "pzavolinsky",
        "ericanderson",
        "DovydasNavickas",
        "theruther4d",
        "guilhermehubner",
        "ferdaber",
        "jrakotoharisoa",
        "pascaloliv",
        "hotell",
        "franklixuefei",
        "Jessidhia",
        "saranshkataria",
        "lukyth",
        "eps1lon",
        "zieka",
        "dancerphil",
        "dimitropoulos",
        "disjukr",
        "vhfmag",
        "priyanshurav",
        "Semigradsky",
        "mattpocock"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    },
    {
      "name": "tonic-ui__react",
      "kind": "edit",
      "files": [
        {
          "path": "types/tonic-ui__react/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "derekhawker",
        "ZachLegros",
        "georgids9"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "use-subscription",
      "kind": "edit",
      "files": [
        {
          "path": "types/use-subscription/use-subscription-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "eps1lon"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "stale",
      "reviewer": "eps1lon",
      "date": "2025-05-12T15:15:33.000Z",
      "abbrOid": "71b0db1"
    },
    {
      "type": "approved",
      "reviewer": "Toliak",
      "date": "2025-05-05T12:01:12.000Z",
      "isMaintainer": false
    },
    {
      "type": "approved",
      "reviewer": "zerosrat",
      "date": "2025-04-20T08:00:40.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "aUsABuisnessman",
      "date": "2025-04-08T03:17:34.000Z",
      "abbrOid": "c0228af"
    }
  ],
  "mainBotCommentID": 2781685597,
  "ciResult": "pass"
}

typescript-bot avatar Apr 06 '25 21:04 typescript-bot

πŸ”” @zerosrat @AdrianMrn @alloy @maraisr @edvinerikson @levibuzolic @ggascoigne @gargroh @riceboyler @johnnyreilly @bbenezech @pzavolinsky @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @eps1lon @zieka @dancerphil @dimitropoulos @disjukr @vhfmag @priyanshurav @Semigradsky @mattpocock @derekhawker @ZachLegros β€” please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

typescript-bot avatar Apr 06 '25 21:04 typescript-bot

I'm currently checking out an issue where memo(Component) does not apply memo typings to props.

arthurgeron avatar Apr 06 '25 22:04 arthurgeron

Dropping memo changes for now, some implementations would make TypeScript reach its limit with the orignal implementation, throwing Expression produces a union type that is too complex to represent.

arthurgeron avatar Apr 07 '25 03:04 arthurgeron

@aUsABuisnessman Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

typescript-bot avatar Apr 11 '25 06:04 typescript-bot

Re-ping @AdrianMrn, @cameron-mcateer, @TiuSh, @Toliak, @alloy, @maraisr, @edvinerikson, @levibuzolic, @ggascoigne, @gargroh, @riceboyler, @johnnyreilly, @bbenezech, @pzavolinsky, @ericanderson, @DovydasNavickas, @theruther4d, @guilhermehubner, @ferdaber, @jrakotoharisoa, @pascaloliv, @hotell, @franklixuefei, @Jessidhia, @saranshkataria, @lukyth, @eps1lon, @zieka, @dancerphil, @dimitropoulos, @disjukr, @vhfmag, @priyanshurav, @Semigradsky, @mattpocock, @derekhawker, @ZachLegros:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

typescript-bot avatar Apr 18 '25 01:04 typescript-bot

I would review it but no ping on me,huh?

aUsABuisnessman avatar Apr 18 '25 02:04 aUsABuisnessman

@arthurgeron One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

typescript-bot avatar Apr 18 '25 05:04 typescript-bot

@eps1lon

Since this is a breaking change, we can't land it as is and need to come up with a rollout plan.

Yes, absolutely.

It probably needs to be opt-in.

I’d push for mandatory constraints over opt-in, ensuring consistency and catching errors early, since this only brings type constraints to what essentially are the rules of hooks and concept of referential equality, aligning with React best practices. Developers can use type assertions or ts-ignore for rare exceptions. As a maintainer I'd be relived to see dozens of type errors that point out to a real issues in the codebase.

Have you tested this on existing codebase to check how many issues it finds?

Yes, both small and large. I’ve just pushed a last update to fix a specific error (TS4023) I found while testing on a codebase.

arthurgeron avatar Apr 18 '25 17:04 arthurgeron

@arthurgeron The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds that are failing do not end up on the list of PRs for the DT maintainers to review.

typescript-bot avatar Apr 20 '25 04:04 typescript-bot

~Placing this back into draft while I take a look at the failed tests.~

arthurgeron avatar Apr 20 '25 04:04 arthurgeron

@eps1lon, @aUsABuisnessman Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

typescript-bot avatar Apr 20 '25 08:04 typescript-bot

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @arthurgeron.

(Ping @AdrianMrn, @cameron-mcateer, @TiuSh, @captbaritone, @alloy, @maraisr, @edvinerikson, @levibuzolic, @ggascoigne, @gargroh, @riceboyler, @johnnyreilly, @bbenezech, @pzavolinsky, @ericanderson, @DovydasNavickas, @theruther4d, @guilhermehubner, @ferdaber, @jrakotoharisoa, @pascaloliv, @hotell, @franklixuefei, @Jessidhia, @saranshkataria, @lukyth, @eps1lon, @zieka, @dancerphil, @dimitropoulos, @disjukr, @vhfmag, @priyanshurav, @Semigradsky, @mattpocock, @derekhawker, @ZachLegros, @georgids9.)

typescript-bot avatar Apr 24 '25 18:04 typescript-bot

bump

arthurgeron avatar May 12 '25 12:05 arthurgeron

@arthurgeron One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

typescript-bot avatar May 12 '25 15:05 typescript-bot

@eps1lon

  • migration docs (what can and can't we codemod?)

This PR doesn't require migrations, it enforces the rules of hooks and exposes real problems in codebases, since we add type wrappings to returned objects from useMemo, useCallback and useState, it knows precisely if an object passed in dependency arrays is properly memoized or not, there's no in-between; if it's flagged it's because it has an codebase issue, not a migration issue, nor subjective or something to fix with codemods imho.

The most outlandish case a codemod could look for are static objects accidentally included in dependency arrays, at most.

  • patterns it flags that should be fine

Assuming comprehension of useMemo/useCallback, none. Let me explain. If someone uses a static, complex/non-primitive variable, declared outside React scope, passed in a dependency array, it'll be flagged. Since it doesn't make sense to add variables from outside React scope to dependency arrays, it's basically a non-issue.

  • patterns it misses

Due to TS limitations with how React types are exported via a "Namespace", memo isn't going to enforce RoH logic on type-level (i.e. compared props should be static or memoized)

  • a case study showing the value it provides (e.g. how often did it find real issues?)

The value of following these is not having your hooks misbehave, which has been widely explained and discussed before, it precisely enforces the default knowledge and rules of hooks already stablished by React on their Rules of Hooks.

  • how it interacts with the React Compiler (e.g. what are patterns already covered by the Compiler, where are advantages of the Compiler, where are advantages of these type changes)

This focuses on types while React Compiler is a build-time optimization tool, it's not related. React Compiler isn't going to be a silver bullet and optimizations will most likely allow for being optionally disabled by users. RC doesn't discard the necessity of understanding and enforcing rules of hooks, this simply brings the rules of hooks to the type level, as per the PR's description and given documentation. It's worth noting that React Compiler:

  • Isn't competing on a replacement to this. While it does auto-optimize things, it doesn't completely relinquish the knowledge or caring for the basic of the rules of hooks.
  • Is still ways away from RC and has no official stable release date
  • Most React projects on production aren't using RC.

Everything here is well described in the PR's description, this isn't any outlandish concept, it's React's, the purpose is not only to bring the existing concept to type-level, but to also avoid people from forgetting it exists. If there's a specific part of the description or docs that's not clear, please don't refrain from pointing out so I can clarify.

arthurgeron avatar May 13 '25 11:05 arthurgeron

This PR doesn't require migrations

Isn't the point to catch accidentally passing unmemoized objects? In those cases people do need to migrate to make the new types happy, right?

eps1lon avatar Jun 10 '25 10:06 eps1lon

This PR doesn't require migrations

Isn't the point to catch accidentally passing unmemoized objects? In those cases people do need to migrate to make the new types happy, right?

The script would have to:

  • add useMemo to dynamically generated objects, or move them outside React’s scope
  • add useCallback to dynamically generated functions, or move them outside React’s scope
  • Not use static references in dependency arrays

For us to fix that, we'd have to:

  • Detect objects/methods that do not depend on anything from the React scope, and move them outside components/hooks so they become static
  • Detect dynamic objects and methods used in React's native hooks, wrap them in useMemo/useCallback, detect objects used within those expressions/methods, add them to dependency arrays, and recursively do the same check for each dependency included in the array.

Yes, it can be done, and if you agree those migrations make sense I'd be more than happy to create the migration script(s); But does this PR have any real chance of being merged?

The migration script is not some effortless task; it'd be a waste to work on it just for this PR to never be merged. I'd feel much safer working on the migration script once there's a clear consensus amongst maintainers.

arthurgeron avatar Jun 13 '25 14:06 arthurgeron

Proofing out a codemod is a requirement for this PR to get merged. Otherwise a lot of codebases will be left behind if they can't reasonably upgrade.

Though this PR hasn't addressed what happens in a codebase with the React Compiler. Those codebases would need an opt-out. This is why I suggested redesigning this to work as an opt-in instead so that people can try it out first.

eps1lon avatar Jul 08 '25 10:07 eps1lon

It's been a month since the last activity on this PR. I'm going to close it since the bot doesn't seem to know the current state.

sandersn avatar Aug 08 '25 15:08 sandersn