Dynamic children break the component

[jonathandewitt-dev]

Initial checklist

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

Affected packages and versions

react-markdown

Link to runnable example

No response

Steps to reproduce

Occasionally, through some whimsical magic, React will choose to render a single string passed to children as an array containing one string. I can't seem to reproduce this scenario reliably, it just happens at what seems like random times.

This being the case, however, means that there are valid markdown strings being passed as children which this component is not resilient enough to handle. I keep getting variations of the error message:

Unexpected value `test` for `children` prop, expected `string`

Where "test" was simply my text string passed as the sole child to the Markdown component. Upon inspection, I was able to discover that my string was being converted to string[] and then rejected by the package. Not a great experience because there's quite literally nothing that can be done within my control when facing this situation.

I was able to fix the problem by temporarily modifying this library's code in my node_modules, from this:

  if (typeof children === 'string') {
    file.value = children
  } else {
    unreachable(
      'Unexpected value `' +
        children +
        '` for `children` prop, expected `string`'
    )
  }

to this:

  const allChildrenAreStrings = Array.isArray(children) ? children.every((child) => typeof child === 'string') : typeof children === 'string';

  if (allChildrenAreStrings) {
    file.value = Array.isArray(children) ? children.join('\n') : children
  } else {
    unreachable(
      'Unexpected value `' +
        children +
        '` for `children` prop, expected `string`'
    )
  }

Expected behavior

It should handle string[] children by confirming that each child is indeed of type string and then Array.join('\n') them to continue processing the markdown.

Actual behavior

For string[] arrays, it reports an error and crashes the app.

Runtime

No response

Package manager

No response

OS

No response

Build and bundle tools

No response

[wooorm]

Hi!

children is the value you pass in. Sometimes through JSX. But it’s your choice how to pass it. You can pass weird things: <Markdown>{1}{2}{3}</Markdown>.

It will always be a simple string, if you are explicit, like is shown in the readme: <Markdown children={`# some string`} /> or <Markdown>{`# some string`}</Markdown>.

[jonathandewitt-dev]

Hey there 👋

So I think there's a misunderstanding here. The string value we pass in is certainly not always interpreted as a string, which is why I'm saying there is valid markdown text not supported by this package. We have no control over how our simple strings get parsed into JSX under the hood.

Here's some evidence I'm telling God's honest truth. 🙂

the type being passed:

the error:

the type evaluated at runtime:

the runtime type after it's been transformed by React/JSX:

[jonathandewitt-dev]

On a side note, I don't think it's adding all that much ambiguity to accept a list of strings, regarding #855 (comment)

Due to the way JSX works, I think it would be perfectly acceptable to support mapped children (although I don't know why you would need to map over children, but it seems like something we shan't judge if it's easy enough to permit it.) As long as the array passed in is checked to be sure each item in the array is only a string, I don't see how it could introduce much confusion.

[wooorm]

(edit: sorry, pressed enter too fast, done:)

The string value we pass in is certainly not always interpreted as a string

which is why I'm saying there is valid markdown text not supported by this package.

I strongly doubt the first statement.

We have no control over how our simple strings get parsed into JSX under the hood.

Yes, I strongly believe that you do.

Here's some evidence I'm telling God's honest truth. 🙂

Your evidence is: TypeScript says it’s so.
But TypeScript lies. TypeScript isn’t real. It’s an approximation of reality. Which is sometimes different from reality.

My evidence to contradict you:

1. billions of people use this package, and do not experience what you see.
2. you cannot create a reproduction of this problem

Unhandled runtime error

Can you please wrap your code to throw some error if props.markdown is not a string first?

Can you otherwise make a reproduction?

[jonathandewitt-dev]

Your evidence is: TypeScript says it’s so.
But TypeScript lies. TypeScript isn’t real

That is not the totality of my evidence. See the final two screenshots. Runtime doesn't lie.

[jonathandewitt-dev]

Here's some additional evidence if you're still not convinced.

[wooorm]

Right, thanks. I also mentioned some other points though. Why would nobody else see this? Why could you not make a reproduction? My next assumption, as nobody else sees this, is that you have an uncommon JSX transform. Or a custom JSX runtime. Are there other things you can do to make the problem occur or disappear?

[jonathandewitt-dev]

Rest assured, I'm desperately trying to identify the root cause, because I find it as bizarre and unbelievable as you do. I'll let you know if and when I identify what's different about my scenario that confuses the transformer and causes it to become an array. There's nothing special about my project, it's just an ordinary Next.js project with all the default babel transforms OOTB.

I will also say this is not the first time I've encountered this (intermittent) error, but it's the first time I decided to contribute rather than hack around it or shop around for a different package - which is what I assume is happening for others, hence the silence around the matter.

Putting aside the reproduction, (which I am actively attempting to isolate on the side,) I wonder more about the reasoning behind the resistance to the change? Do you believe accepting string[] is going to introduce problems for users?

[wooorm]

There’s lots of things that can be thought up, such as: bugs need to be squashed, not shoved under the rug. Stuff like that.
Any bug that cannot be reproduced is highly suspicious. Any code that is not needed, or cannot be tested correctly, cannot be maintained well, is a problem for users.
I don’t want to maintain code in 10 years that I do not understand.

[wooorm]

Thanks for more info. So you don’t have dynamic children, which is what this issue is about. Your problem seems to be Parcel being broken: #747 (comment). Hiding these comments so we can focus on the OP.

[jonathandewitt-dev]

@wooorm

Alright, I figured it out. It's not an intermittent JSX behavior after all. It's due to my package react-shadow-scope emulating DSD with React.Children.map() here. All I need to do is add something like resultChildren.length === 1 ? resultChildren[0] : resultChildren and it works.

So I admit it, it's an upstream issue, not a problem with JSX or this package. 🙂

(Though I still think it's a harmless addition that may avoid such weirdness in the future, but that's your discretion)

[wooorm]

Thanks but no thanks!