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

Make this project agnostic to runtime environment (node/browser) #92

Open IanEdington opened 1 year ago

IanEdington commented 1 year ago

A bit of a crazy suggestion, but could this project depend on mjml-browser instead of mjml? Are there huge benefits or disadvantages in this choice?

The mjml-browser plugin mentions 2:

I think both points are mostly covered by the fact that this library uses react. We can make out own react based components, same goes for mj-include.

Originally posted by @Bertg in https://github.com/Faire/mjml-react/issues/89#issuecomment-1434875793

related issues:

Bertg commented 1 year ago

One of the big downsides of using mjml-browser is that that MjmlInclude isn't possible. However, it doesn't make a lot of semantic sense to have that in a react setup anyway.

What I thought we could potentially do, to alleviate this, is introduce something like this:

<MjmlImport src="./some/file.mjml" />

This tag would use the native import capabilities of node (or webpack in Storybook mode) to import the mjml file raw. This could then be passed "as is" to the mjml2Html step.

This makes is distinct enough to not be confused with mj-include while still offering all the benefits from reusing older partials.

IanEdington commented 1 year ago

I'm of the opinion that we should just remove the render to mjml function all together and add this as a suggestion to the getting started.

  1. This is better for users of the library because they don't have to question the magic of what's happening in the library and can add their own steps between each part more easily.
  2. This makes the library browser/node agnostic.
  3. It makes it clear in the render function that you need to choose between mjml and mjml-browser.
import { renderToMjml } from "@faire/mjml-react/utils/renderToMjml";
// import mjml2html from "mjml";
// import mjml2html from "mjml-browser";
import { MJMLParseResults } from "mjml-core";
import React from "react";

export function render(email: React.ReactElement): MJMLParseResults {
  return mjml2html(renderToMjml(email));
}
Bertg commented 1 year ago

@IanEdington In an other thread I've already indicated that this may not be the best corse of action.

But maybe this is an opportunity in disguise. If you want to lift the render method from the package, where does it land? Maybe there is room for react-mjml-render as a companion package? This package could inherently know how to render given it's in a node environment or not. Having both mjml and mjml-browser as optional dependencies.

IanEdington commented 1 year ago

Some unrelated thoughts:

I like the idea of a package for rendering but it's outside the scope of what we're able to do with this package for now. One way we could move towards a rendering package would be to collect example renderers for different situations in a community driven section of the repo. Then once we've collected a number of examples and see the interest it would justify working more on this idea.

If you decide to write a package for this multi-environment renderReactToMjml I'd be happy to answer any questions.

IanEdington commented 1 year ago

Should we change the title of this issue to:

"Make this project runtime environment agnostic (node/browser)"

I think that would capture the part we're aligned on accomplishing. It seems like the idea of using mjml-browser would break too many other implementations but sticking with mjml is making browser environments 😭, and trying to do both is a recipe for lots of future maintenance.

If we're in agreement on that path forward we should be able to make these changes in the v4 alpha relatively soon

pachuka commented 5 months ago

Just as an FYI, and not sure if it's still relevant, but the old mjml-react @ 2.0.7 used to work just fine in a browser-only environment (I have a repo that does just that). I had left a comment on the following bug thread https://github.com/wix-incubator/mjml-react/issues/84#issuecomment-1218657525 but it might have been missed, but specifically the minimize css process that was added was what broke the browser compat:

Being able to use react-mjml in a browser enviornment only breaks after the minify changes were introduced in the latest release (fine in 2.0.7, doesn't work in 2.0.8) as part of https://github.com/wix-incubator/mjml-react/pull/75.

Not sure if you all are still using that minification process.

emmclaughlin commented 5 months ago

Hi @pachuka, we made this project have as few dependencies as possible for that exact reason. As a result there is no minifier in the dependencies of the latest version of @faire/mjml-react (package.json).

In our getting started section we have a brief section on using mjml vs mjml browser. We kept the render function in utils as an example/lightweight starting option and also to not break any existing uses. However we also made it completely possible for the user to ignore it and use their own render function (which can be with mjml-browser and does not need to be minified).

So the summary is we removed the dependency on that minification process so that you only have to use it if you want.

jlarmstrongiv commented 4 months ago

Is there a renderer that works in web workers? window is not defined in web workers either

emmclaughlin commented 4 months ago

@jlarmstrongiv web workers isn't something we have looked into, so unfortunately I am not aware of the best options in that case.