ViewType caption typing is incorrect

[zackdotcomputer]

Because of how Captions are rendered by default, the typing of caption as ReactNode is incorrect. This includes object types like arrays and React.Component types. If you try passing one of these as the caption, however, it renders as [Object object] rather than rendering properly.

What it should be instead is string | number | boolean | null | undefined - the primitive types that react can render. I'll open a PR to fix.

It would be great if we got support in the future for fully dynamic captions using React Components, but for now this is a much quicker fix.

[davwheat]

We do also support HTML elements as of 1.1.3 (if I set that up correctly). I'm not very familiar with typing, to be honest, but if there's a way to state that, that would be fab.

[zackdotcomputer]

Hey @davwheat - thanks for the reply.

Your work does support HTML strings correctly, but the bug here is that the ReactNode type also includes a bunch of object types like React Component subclasses that aren't supported. The issue stems from the line in Footer.js where you're rendering the caption:

{ParseHtml(`<span>${caption}</span>`)} // Footer.js:105

Because the argument to ParseHtml is using the string interpolation, caption is going to be coerced to a string by Javascript. If you pass in a JSX tag, this line then calls .toString and it becomes "[Object object]". (Additionally, ParseHtml can't take a ReactNode object anyways, it expects a string itself.)

My PR #368 is intended as a bandaid over this until the line in Footer.js can be fixed. Ideally, we'd move it so that the footer's caption is rendered kinda like this:

{(caption instanceof string) ? ParseHtml(`<span>${caption}</span>`) : caption}

This way we'd render using ParseHtml if it was a string or an HTML rendered string, but we'd just hand it off to JSX to render if it was any other type. I didn't want to make that change as my first PR, though, cause I'm still learning how your codebase is laid out and wasn't sure if it would have unexpected consequences.

[davwheat]

It shouldn't have any issues changing it to that. This isn't my project and, to be honest, I don't even understand how this codebase is laid out! 😅

Previously, that line was just {caption}, do making it what you suggested in your last comment would be fine.

The change to using parseHtml was made in the last version and I realised that was what broke the original support for passing React directly to the footer.

PS: I forgot to say thank you for the PR, so... thank you!

[zackdotcomputer]

Oh zero worries!

I agree it sounds like I can safely make this the smarter if/else render. Will update the PR with that.

[florianuphoff]

Hey, we ran into another problem with ParseHTML.

The package is declared globally which uses the document unsafetly. Using the current version of react-iamges affects SSR usage. Any package which includes react-images cannot be used in a SSR app.

There will be another PR to fix this issue soon. We are working on it.
Long story short: you can render HTML without using the parseHTML package. This way, SSR is going to work again.

[zackdotcomputer]

@florianuphoff Interesting - I'm using it for a SSR here and I'm not seeing any issue with it? Can you open a new issue with the problem you're seeing (and so we can track this separately from this issue which I think we're about to close)?

[florianuphoff]

@zackdotcomputer Sure - my coworker is going to create one.
I guess we have a special case because we use react-images in a bundled package which we include in a SSR app. Maybe it's a problem with webpack or something else. Further information will follow 👍