Skip to content
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

react-markdown@9.0.3 causes a significant increase in memory usage #883

Closed
4 tasks done
noahwaldner opened this issue Jan 27, 2025 · 6 comments · Fixed by #893
Closed
4 tasks done

react-markdown@9.0.3 causes a significant increase in memory usage #883

noahwaldner opened this issue Jan 27, 2025 · 6 comments · Fixed by #893
Labels
🤷 no/invalid This cannot be acted upon 👎 phase/no Post cannot or will not be acted on

Comments

@noahwaldner
Copy link

noahwaldner commented Jan 27, 2025

Initial checklist

Affected package

react-markdown@9.0.3

Steps to reproduce

  1. Use react-markdown in a project
  2. Run a typecheck with the --generateTrace option
  3. Analyze the trace with @typescript/analyze-trace
Check file .../markdown.tsx (3388ms)
└─ Check deferred node from (line X, char X) to (line X, char X) (3341ms)
   └─ Compare types 1971 and 1260 (3331ms)
      └─ Compare types 1971 and 1247 (3327ms)
         └─ Compare types 338 and 1946 (3327ms)
            └─ Compare types 336 and 1261 (3327ms)

Actual behavior

After upgrading react-markdown, TypeScript compilation shows significantly increased memory usage and type checking time. The TypeScript trace shows extremely long type comparison times (3300+ ms) when processing files that import react-markdown.

This appears to be related to the recent change from @typedef to @import in the type definitions.

Previously @typedef {import('hast').Element} Element was handled as a simpler JSDoc annotation.

@import {Element} from 'hast' forces TypeScript to:

  • Load and process full type definitions from each imported package
  • Maintain more complex type relationships in memory
  • Perform deeper type analysis during comparisons

The change from JSDoc annotations (@typedef) to TypeScript imports (@import) represents an implementation change that affects build performance for consumers.
Such changes should ideally be released as a minor version bump rather than a patch version, since patch versions should only contain backwards compatible bug fixes.

Expected behavior

TypeScript should compile with similar resource usage and performance as previous versions.

Runtime

node@22.13.1

Package manager

pnpm@9.15.4

Operating system

macOS Sequoia 15.1.2

Build and bundle tools

No response

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jan 27, 2025
@wooorm
Copy link
Member

wooorm commented Jan 27, 2025

Sounds like an issue with TypeScript? Why not report it there?

@noahwaldner
Copy link
Author

noahwaldner commented Jan 27, 2025

The performance regression appears to be due to how the type relationships are being constructed, rather than a TypeScript issue itself.

In the original implementation:

 /** @typedef {import('hast').Root} Root */
/** @typedef {import('unist-util-visit').BuildVisitor<Root>} Visitor */

/** @type {Visitor} */
function transform(node, index, parent) {

The complex type relationship between BuildVisitor and Root is resolved once at the typedef level.

In the new implementation:

/** @import {BuildVisitor} from 'unist-util-visit' */
/** @import {Root} from 'hast' */

/** @type {BuildVisitor<Root>} */
function transform(node, index, parent) {

TypeScript needs to resolve and compare these types each time the function used/referenced. This means that everytime, it needs to:

  • Resolve the imported BuildVisitor type
  • Resolve the imported Root type
  • Construct the generic type BuildVisitor
  • Compare this constructed type with where it's being used

This leads to the deep type comparison traces we're seeing:

CopyCompare types 1971 and 1260 (3331ms)
└─ Compare types 1971 and 1247 (3327ms)
   └─ Compare types 338 and 1946 (3327ms)
      └─ Compare types 336 and 1261 (3327ms)

@remcohaszing
Copy link
Member

Because of the aa5933b, the emitted type definitions no longer contain BuildVisitor. I haven’t investigated deeply, but this can’t be the cause.

$ grep BuildVisitor -r node_modules/react-markdown 
node_modules/react-markdown/lib/index.js: * @import {BuildVisitor} from 'unist-util-visit'
node_modules/react-markdown/lib/index.js:  /** @type {BuildVisitor<Root>} */
$ npm ls react-markdown
└── react-markdown@9.0.3

@wooorm
Copy link
Member

wooorm commented Jan 27, 2025

This means that everytime, it needs to:

I don’t think that this happens; you are looking at JS but your TS is not reading JS. Like remco shows, you do not get the BuildVisitor type

@wooorm
Copy link
Member

wooorm commented Feb 3, 2025

Closing, there is no reproduction, it is very vague what this is, whether something needs to or can be changed here.

@wooorm wooorm closed this as not planned Won't fix, can't repro, duplicate, stale Feb 3, 2025
@wooorm wooorm added the 🤷 no/invalid This cannot be acted upon label Feb 3, 2025

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Feb 3, 2025
remcohaszing added a commit that referenced this issue Mar 1, 2025
The way we used `ElementType` caused a significant increase of resource
usage for TypeScript.

I can’t really explain it, but this fixes it. I’m personally inclined to
say people should avoid using the `JSX` namespace, but I see no other way
around it.

Without this change, autocompletion in my editor takes a noticable time
to load. Now it’s instant.

On my machine this halves the time of running `tsc -b` from ~15s to ~7s.

Closes #883
@ChristianMurphy ChristianMurphy marked this as a duplicate of #895 Mar 2, 2025
wooorm pushed a commit that referenced this issue Mar 3, 2025
Related-to: GH-895.
Related-to: GH-883.

Closes GH-893.

Reviewed-by: Christian Murphy <christian.murphy.42@gmail.com>
Reviewed-by: Titus Wormer <tituswormer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤷 no/invalid This cannot be acted upon 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging a pull request may close this issue.

3 participants