TypeError when using react-syntax-highlighter

Katsukiniwa commented 2 years ago

Initial checklist

[X] I read the support docs
[X] I read the contributing guide
[X] I agree to follow the code of conduct
[X] I searched issues and couldn’t find anything (or linked relevant results below)

Affected packages and versions

7.1.1

Link to runnable example

No response

Steps to reproduce

type error happened.

import ReactMarkdown from 'react-markdown';
import { Prism as SyntaxHighlighter } from 'react-syntax-highlighter';
import { darcula } from 'react-syntax-highlighter/dist/cjs/styles/prism';

const sample = () => {
  return (
    <ReactMarkdown
      components={{
        code({ node, inline, className, children, ...props }) {
          const match = /language-(\w+)/.exec(className || '');
          // I avoid with type casting like this.
          // const ref = props.ref as LegacyRef<SyntaxHighlighter> | undefined;

          return !inline && match ? (
            <SyntaxHighlighter style={darcula} language={match[1]} {...props}>
              {String(children).replace(/\n$/, '')}
            </SyntaxHighlighter>
          ) : (
            <code className={className}>{children}</code>
          );
        },
      }}
    >
      {post.content}
    </ReactMarkdown>
  )
}

Expected behavior

props.ref type should be LegacyRef<SyntaxHighlighter> | undefined

Actual behavior

no error found.

Runtime

Node v16

Package manager

No response

OS

macOS

Build and bundle tools

Next.js

ChristianMurphy commented 2 years ago

@Katsukiniwa thanks for reaching out, sorry you ran into a spot of trouble. This isn't a bug in react-markdown it has to do with how @types/react-syntax-highlighter handles additional props being passed through {...props}, specifically ref.

see https://codesandbox.io/s/react-markdown-syntax-highlight-typescript-p9tw0 for an example, try adding and removing ref on line 25 to see the difference it makes.

code({ node, inline, className, children, ref, ...props }) {
                                          ^^^^

full example

````tsx import React from "react"; import Markdown from "react-markdown"; import { Prism as SyntaxHighlighter } from "react-syntax-highlighter"; import { darcula } from "react-syntax-highlighter/dist/cjs/styles/prism"; const markdownSource = ` # heading * list * items \`\`\`js function () {} \`\`\` `; const App = () => ( {String(children).replace(/\n$/, "")} ) : ( {children} ); } }} > {markdownSource} ); export default App; ```

Katsukiniwa commented 2 years ago

@ChristianMurphy Thank you, it worked!! The sample code on the README.md, ref is not passed as argument. Is this a wrong something? If it is something bug, may I fix it and create pull reqeuest?

https://github.com/remarkjs/react-markdown#use-custom-components-syntax-highlight

ChristianMurphy commented 2 years ago

The example in the readme is JavaScript. Passing ref doesn't cause a runtime error, just a type(script) error.

Thoughts @wooorm or @remcohaszing on changing the example to TS? 💭

remcohaszing commented 2 years ago

I think this could be a bug in react-markdown. This depends on how code() is called. I’m not very familiar with this particular package.

On the following lines JSX.IntrinsicElements is used to get prop types.

https://github.com/remarkjs/react-markdown/blob/9a25ef80daeb72b0740e756cb126527fe85f932f/lib/ast-to-react.js#L52-L58

This is correct if code is called as a function, but incorrect if it’s called as React JSX. In that case the types should be:


/**
 * @typedef {React.ComponentPropsWithoutRef<'code'> & ReactMarkdownProps & {inline?: boolean}} CodeProps
 */

ChristianMurphy commented 2 years ago

It is rendered here: https://github.com/remarkjs/react-markdown/blob/9a25ef80daeb72b0740e756cb126527fe85f932f/lib/ast-to-react.js#L316-L319 and is reflecting the intrinsic type here: https://github.com/remarkjs/react-markdown/blob/9a25ef80daeb72b0740e756cb126527fe85f932f/lib/complex-types.ts#L24-L28

and I correct in interpreting your comment to mean

// in react-markdown/lib/complex-types.ts line 27
JSX.IntrinsicElements[TagName]
// could be replaced with
React.ComponentPropsWithoutRef<TagName>

?

remcohaszing commented 2 years ago

That seems correct more than the types I referenced, but indeed the problem is what I suspected: JSX.IntrinsicElements is used where React.ComponentPropsWithoutRef should be used.

ChristianMurphy commented 2 years ago

@Katsukiniwa you could also give version 7.1.2 a try, it should not require any tinkering with ref in typescript.

Katsukiniwa commented 2 years ago

@ChristianMurphy Thank you !!!

18601673727 commented 2 weeks ago

If I'm reading this issue correctly, at this momenet, on version 9.0.0, the same problem is still there and README.md is still misleading people

ChristianMurphy commented 2 weeks ago

Hey again @18601673727! 👋 You aren't reading the issue correctly. The example in the readme is js, not ts.

And the compatability issue in the types was from several react versions ago, you are seeing something something relayed but different.

Likely you are using an incompatible version of the react types. Version 18 is currently supported.

https://github.com/remarkjs/react-markdown/blob/78160f5a0877675c1c18417a220f9948de143720/package.json#L96

18601673727 commented 2 weeks ago

Hello @ChristianMurphy, sorry being so blunt about it, but my current package.json is here:

  "dependencies": {
    "@ethereumjs/util": "^9.0.3",
    "@metamask/providers": "^16.0.0",
    "@nextui-org/react": "^2.3.6",
    "@nextui-org/system": "2.1.2",
    "@nextui-org/theme": "2.2.3",
    "@prisma/client": "5.13.0",
    "@types/node": "20.5.7",
    "@types/react": "18.3.1",
    "@types/react-dom": "18.3.0",
    "autoprefixer": "10.4.19",
    "clsx": "^2.0.0",
    "date-fns": "^3.6.0",
    "eslint": "8.48.0",
    "eslint-config-next": "14.2.1",
    "framer-motion": "^11.1.7",
    "i18next": "^23.11.2",
    "intl-messageformat": "^10.5.0",
    "md-editor-rt": "^4.13.5",
    "next": "14.2.3",
    "next-auth": "5.0.0-beta.16",
    "next-remove-imports": "^1.0.12",
    "next-themes": "^0.3.0",
    "postcss": "8.4.38",
    "posthog-js": "^1.129.0",
    "react": "18.2.0",
    "react-dom": "18.2.0",
    "react-hot-toast": "^2.4.1",
    "react-markdown": "^9.0.1",
    "react-nice-avatar": "^1.5.0",
    "react-syntax-highlighter": "^15.5.0",
    "remark-breaks": "^4.0.0",
    "remark-gfm": "^4.0.0",
    "safe-marked": "^16.0.0",
    "sharp": "^0.33.3",
    "tailwind-merge": "^2.3.0",
    "tailwind-variants": "^0.2.1",
    "tailwindcss": "3.4.3",
    "typescript": "5.4.5",
    "zod": "^3.22.5",
    "zod-i18n-map": "^2.27.0",
    "zod-validation-error": "^3.2.0"
  },
  "devDependencies": {
    "@tailwindcss/typography": "^0.5.13",
    "@types/react-syntax-highlighter": "^15.5.12",
    "prisma": "^5.13.0",
    "ts-node": "^10.9.2"
  }

I know all this open sourced "compiler work" weights a lot, and maintainers like you deserve huge amount of respect, but we all know that it's 2024 already and typescript is more or less a default kind of thing to README.md about, upstream issue happens but considering the user base react-markdown has of now, we might be obliged to handle theirs shortcomings. So it's either modify README.md on this side or don't recommand that react-syntax-highlight no more. Or we would expect this kind of problem happen again.

ChristianMurphy commented 2 weeks ago

we all know that it's 2024 already and typescript is more or less a default kind of thing to README.md about

While I like and use TypeScript, I respectfully disagree. JavaScript is still the go to for documentation. It is in:

The React docs https://react.dev/
The Node docs https://nodejs.org/docs/latest/api/
The Vite docs https://vitejs.dev/
The Remix docs https://remix.run/
The Gatsby docs https://www.gatsbyjs.com/docs/tutorial/getting-started/part-5/
and many more

So it's either modify README.md on this side or don't recommend that react-syntax-highlight no more. Or we would expect this kind of problem happen again.

The third, and best option, is to look into the root of the problem, and open a fix to @types/react or react-syntax-highlight. This is a types regression that happened elsewhere, this has worked fine for three years.

I know all this open sourced "compiler work" weights a lot, and maintainers like you deserve huge amount of respect, but [...]

No "but". This isn't the place or the time to make demands or state your opinion as a fact. We're working to help you and the community as a whole, rushing incomplete half fixes with incomplete information doesn't help.

Take the time to frame your problem https://github.com/remarkjs/.github/blob/main/support.md, share a runnable example of the issue you are seeing (https://stackblitz.com/github/remarkjs/.github/tree/main/sandbox-templates/react-markdown-with-vite is good starting point), and consider the alternatives.

18601673727 commented 2 weeks ago

Are you saying "@types/react": "18.3.1", is not compatible enough for "@types/react": "^18.0.0" as per you suggested? Why my VSCode is still complaining with that piece of code your README.md documented? All this is a simple coincidence or ignorance/arrogance? As a member of 12k-star opensource project, bothering his downstream users by not willing to make a teeny tiny modification in the documentation then lecturing people how and when to communicate? You know what, don't do any changes, leave it as it is. 👎

ChristianMurphy commented 2 weeks ago

Are you saying "@types/react": "18.3.1", is not compatible enough for "@types/react": "^18.0.0" as per you suggested?

Nowhere did I say that, making things up just to argue will get you blocked from the community.

Why my VSCode is still complaining with that piece of code your README.md documented?

Share a reproducible example of what you are seeing as I asked you to above, and we can find out together.

All this is a simple coincidence or ignorance/arrogance?

Likely, right now you seem to be both. Work with the people trying to help you instead of fighting them.

As a member of 12k-star opensource project, bothering his downstream users by not willing to make a teeny tiny modification in the documentation then lecturing people how and when to communicate?

I'm not willing to risk breaking code or misleading 12k+ people because one angry commenter, who refuses give an example of what they are seeing, disagrees with part of the documentation. Show the problem, discuss what you have tried, and we can discuss solutions.

wooorm commented 2 weeks ago

@18601673727 you are embarrassing yourself. This is issues. You are commenting on other people's issues and picking fights. Last warning. Be constructive and follow orders or be blocked.

remarkjs / react-markdown