Faire / mjml-react

React component library to generate the HTML emails on the fly
https://www.npmjs.com/package/@faire/mjml-react
MIT License
375 stars 16 forks source link

feat(utils): allow object props to make it to the component #99

Closed robin-ray closed 1 year ago

robin-ray commented 1 year ago

Hello!

Summary

This PR changes the behavior of convertPropValueToMjml to give users of this library more flexibility, at the expense of allowing some unintentional mistakes to happen (ex: accidentally rendering some-prop="[Object object]").

Motivation

I have built a small library that uses objects and toString() to build up expressions that can be rendered to an MJML template. This works great for props on ordinary React HTML elements (this is a contrived example):

const MyImage = () => {
  const imgURL = { toString: () => "/path/to/some/resource" };
  return <img src={imgURL} />;
};

React renders <img src="/path/to/some/resource" /> for the snippet above.

However, I have to explicitly convert the value to a string for it to pass through mjml-react:

const MyImage = () => {
  const imgURL = { toString: () => "/path/to/some/resource" };
  return <MjmlImage src={`${imgURL}`} />;
};

Without the explicit conversion, React will render <mj-image /> because convertPropValueToMjml does not allow object props through (except for dangerouslySetInnerHTML). It would be convenient if mjml-react components behaved more like React HTML components in this regard so that I don't have to do the extra string conversion.

Thanks for considering this PR!

emmclaughlin commented 1 year ago

Hi @robin-ray, and thank you for the idea.

Unfortunately I am not sure giving more flexibility at the cost of unintentional mistakes is something we want to do with regards to the type safety on the mjml-react components. However, it seems as though you are using javascript? I noticed that the src prop on the html img tag also throws a type error when trying to pass the imgURL object.

Essentially what I am thinking is it may be reasonable to relax checks in convertPropValueToMjml so that all props can pass through in javascript, but in typescript

const MyImage = () => {
  const imgURL = { toString: () => "/path/to/some/resource" };
  return <img src={imgURL} />;
};

will still throw an error.

robin-ray commented 1 year ago

That's right! I'm using JavaScript for my components to avoid that issue.

robin-ray commented 1 year ago

I'm all for having this be a compile-time error in TypeScript! My concern is about the runtime check.

emmclaughlin commented 1 year ago

In that case I completely agree, it is not a good development experience for props to magically disappear. Just running CI checks now

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 3.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: