fix: use stricter type definition for children of <Code>

[bjoerge]

Currently, the <Code> component accepts any valid ReactNode, which means this is works compile time:

<Code><b>Ooops</b></Code>

But breaks runtime, as the Code component expects children to be stringifiable, coerces it to a string runtime and thus render:

[object Object]

This lead to error messages in the studio showing [object Object] instead of the error message, see for example the bug report over at sanity-io/sanity#8712

This fixes the issue by changing the children type to something that can safely be stringified, and also considering that children can be iterable (e.g. <Code>The number is {42}</Code> will make children iterable).

Note: incidentally, the above snipped actually used to work prior to lazy loading refractor (783942a), because before then, we'd only stringify if refractor was loaded and language was passed:

ui/src/primitives/code/code.tsx

Lines 24 to 26 in 7a0231b

	{!(language && registered) && <code>{children}</code>}
	{language && registered && (
	<Refractor inline language={language} value={String(children)} />

This caused inconsistent behavior, so we should stick with always stringifying children.

After this PR, the above code will give a compile error:

error TS2322: Type 'Element' is not assignable to type 'StringifiableNode'.

       <b>Ooops</b>
        ~~~~~~~~~~~~

  code.tsx:27:3
      children: StringifiableNode
         ~~~~~~~~
    The expected type comes from property 'children' which is declared here on type 'IntrinsicAttributes & Omit<CodeProps & Omit<HTMLProps<HTMLElement>, "as" | "size" | "children">, "ref"> & RefAttributes<...>'

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sanity-ui-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 21, 2025 11:30am
sanity-ui-workshop	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 21, 2025 11:30am

[bjoerge]

Thinking some more about this, I'm wondering if we, for now, should instead restore the previous behavior of not stringifying if language is omitted. The inconsistency really feels like a bug to me, but I suspect there are quite a few cases of this breaking in the wild. WDYT @stipsan?

[stipsan]

@bjoerge it seems like the stringify is an oversight:

<LazyRefractor language={language} value={String(children)} />

The previous behavior should be restored by just passing children as is?

<LazyRefractor language={language} value={children} />

[stipsan]

And of course stringify here:

{language && registered && <Refractor inline language={language} value={String(value)} />}

[bjoerge]

Yeah, that should restore the earlier behavior

[EoinFalconer]

Much better testing. I don't know if we want to add tests for the stringifyChildren function?

[bjoerge]

Now that #1620 landed this is less of a pressing issue, but still think we should consider this improvement as it removes an inconsistency leading to confusing behavior.

I start out by writing this:

<Code>
  <b>Some code</b>
</Code>

This outputs Some code in bold, perhaps as expected.

However, if I then add a language:

<Code language="typescript">
  <b>Some code</b>
</Code>

This will now render [object Object] and not give me any hits about what's wrong. Even worse, this is also dependent on whether the selected language is registered with refractor or not, e.g. language="nosuchlang" will revert back to the jsx-rendered output.

To me, this feels like very much like a bug not a feature, and I think the right thing is to always stringify children regardless. I don't see the case for using this component to render anything else than source code, but maybe I'm missing something.

Note: this would now be a breaking change, so updated the commit to reflect that.

[stipsan]

I'm hesitant to do a v3 of @sanity/ui for this, as it's a large library and it'll take time for plugins and even our own tooling to coalesce on it. In the mean time it'll double bundle sizes, which in the case of sanity schema extract commands, bundle commands like next build, significantly add to these.
I'm more inclined to have it rollout in a minor, with a phase where we warn against non-string code, and use the new typings, first. And then rollout the breaking change in a minor.

[bjoerge]

That's fair. I guess you can also argue that it's a bugfix :) I'll adjust the implementation and commit message accordingly (I'm giving this lower priority, so probably won't happen before next week some time).

[justshiv]

@bjoerge would you like to resurrect this PR?