number -> px seems like it could result in a bug

[IanEdington]

MJML already converts numbers into pixels in certain cases. This seems like it could result in a bug where we convert a number to a px for an underlying mjml component that doesn't do the conversion. If possible I'd rather have number be converted to a string instead.

https://github.com/Faire/mjml-react/blob/e8befa5575d62fb3914a1dfc92137dedbf733afb/src/utils/mjml-component-utils.ts#L67-L69

[bradleyayers]

I don't really understand why converting to px is needed? if it's a number then why shouldn't it just be left as a number (or maybe converted to a string, though that isn't done for the boolean case). I think this logic should probably be:

    if (typeof value === "number") {
        if (numberToPixel.includes(name)) {
            return `${value}px`;
        } else {
            return value;
        }
    }

I ran into a nasty issue where previously we were using mjml-react and were using border={0} on some elements, and after upgrading to @faire/react-mjml these silently became no-op attributes, even though the TypeScript types for the border prop permits string or number.

[emmclaughlin]

Looking at this line it seems as though border was included in a prop types override. In general we just pull the prop types from the mjml component definition. If we were to remove that override, border would actually not accept a number, e.g. mj-column in docs is defined as a string.

In my opinion removing the override is probably the best solution, as it will enforce the types to be what mjml wants. So border={0} would cause a type error. In the border case mjml probably converts border={0} to border="0" but I don't think we have a way to programmatically know which props it will convert from number to string (numberToPixel is just an arbitrary definition as far as I know).

[bradleyayers]

I did some experimenting and mj-column seems to support border="0", i.e.:

<mjml>
  <mj-head>
    <mj-attributes>
      <mj-column border="5px solid black"/>
    </mj-attributes>
  </mj-head>
  <mj-body>
    <mj-section>
      <mj-column border="0">
        <mj-text>text</mj-text>
      </mj-column>
    </mj-section>
  </mj-body>
</mjml>

From what I understand the conversion from border={0} to border="0" isn't done in mjml, but rather in React via https://github.com/Faire/mjml-react/blob/main/src/utils/renderToMjml.ts#L5C38-L5C38. When React renders to HTML, all values become strings (because by definition in HTML all tag attributes are strings), so mjml will never see an actual number value, it only receives only strings.

The mjml docs are a little confusing, because in a lot of places it says the unit is string, but I don't think of that as technically being a unit.

Another thought, https://github.com/Faire/mjml-react/blob/dc8cc23ccbd48669d35af59af709211d6aab8901/src/utils/mjml-component-utils.ts#L73 I think it would be safer if this line actually threw an error saying unexpected value, rather than silently swallowing values, I think if a value ends up being a no-op it's should be mjml's responsibility to do that, not mjml-react. But instead of throwing I would probably just pass all the values through to mjml as-is

https://github.com/Faire/mjml-react/blob/main/scripts/generate-mjml-react-utils/getPropTypeFromMjmlAttributeType.ts#L38 I'm guessing this line is trying to make autocomplete better? e.g. so instead of align: string it becomes align: "top" | "bottom" | … so you then get autocomplete when typing in the prop value?

https://github.com/Faire/mjml-react/blob/main/scripts/generate-mjml-react-utils/getPropTypeFromMjmlAttributeType.ts#L44 since this is also allowing number via string | number, that logic will need to be kept in sync with https://github.com/Faire/mjml-react/blob/main/src/utils/mjml-component-utils.ts#L38.

I think all in all I'm leaning towards my patch where the "silently discard numbers" scenario is fixed. I think while removing the type override might be a step in the right direction, there would still be a big foot gun with passing numbers through as prop values.

[emmclaughlin]

Thank you for the detailed investigation here!

I agree that having a silent return is a poor programming experience. I think in the short term just returning the value and letting mjml handle throwing the errors is a good idea. We have had examples in the past of others getting burned by this, e.g. https://github.com/Faire/mjml-react/pull/99.

In the long term, there may still room for improvement though. It would be great to improve the type safety so issues are identified as early as possible. That would go beyond a bug fix, and is still open for more detailed investigation/discussion.