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

Can not get mjml-react to work with Storybook #89

Closed Bertg closed 1 year ago

Bertg commented 1 year ago

I have been trying to get mcml-react (v3) to work nicely with storybook.

An example project, with a fairly clear readme, can be found here: https://github.com/Bertg/mjml-react-storybook-example

The basic issue is that the mjml-react and its dependancies seem to try and use the fs package, which is not available in the storybook runtime.

I create this ticket as a followup of the conversation with @IanEdington from last November.

emmclaughlin commented 1 year ago

Hi @Bertg ! If the issue is that storybook is looking for the fs package you should be able to just add a fallback for webpack >5 using the webpackFinal option in the storybook config https://storybook.js.org/docs/react/configure/overview.

  webpackFinal: async (config, { configType }) => {
    // Make whatever fine-grained changes you need
    // Return the altered config
    return {...config, resolve: {...config.resolve, fallback: {fs: false}}};
  },
emmclaughlin commented 1 year ago

The other thing to consider is the alias of mjml to mjml-browser.

  webpackFinal: async (config, { configType }) => {
    // Make whatever fine-grained changes you need
    // Return the altered config
    return {
      ...config, 
      resolve: {
        ...config.resolve, 
        alias: {mjml: "mjml-browser"}, 
        fallback: { fs: false },
      };
    };
  },
Bertg commented 1 year ago

Ok, ok I'm getting somewhere.

I still can use @faire/mjml-react/utils/render. It loads other libraries like html-minifier, uglify-js , clean-css etc... which seem to be breaking the setup.

However, I did get it to work using @faire/mjml-react/utils/renderToMjml and mjml2html from mjml-browser directly. (I have not pushed this).

It's not super clean, but could be neatly wrapped up into a Readme, or even a dedicated package.

emmclaughlin commented 1 year ago

Ah yes, I think you need to alias uglify-js like so:

const extraAlias = {
  mjml: "mjml-browser",
  "uglify-js": path.resolve(__dirname, "browser-mocks/uglify-js.js"),
};

where the uglify-js.js file is just

module.exports = {};

I think the remaining errors come from htmlMinify using other libraries that break the storybook setup. Perhaps the render function from @faire/mjml-react/utils/render should allow for customizing the htmlMinify configuration as well. We didn't initially look into this much when taking ownership of maintaining mjml-react. We recommend writing your own render function that suits your needs but wanted to keep this one in as an example.

IanEdington commented 1 year ago

👋 Hi @Bertg, Thanks for following up! We would welcome a "getting started with storybook" contribution to the yet to be created docs directory.

@emmclaughlin I wonder if we should just remove html minifier from v4. I know mjml has indicated it wants to move away from minification and it seems like overkill for this library.

Bertg 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.

IanEdington commented 1 year ago

We currently only depend on mjml in the render function: https://github.com/Faire/mjml-react/blob/e8befa5575d62fb3914a1dfc92137dedbf733afb/src/utils/render.ts

I think we could remove mjml as a dependency, requiring people to install mjml seperately to use the render function but we can't change the render function to depend on mjml since that will silently break any emails people have written that have the include tag.

It seems like your problem is with the render function.

Could you try using this file as a substitute to your render function? I think something like this should work even though you have "mjml" installed as a dependency since it won't get imported anywhere in the browser context.

browser-renderer.ts

import mjml2html from "mjml-browser";
import { renderToMjml } from "@faire/mjml-react/utils/renderToMjml"; // might have to tweak this
import { MJMLParsingOptions, MJMLJsonObject, MJMLParseError } from "mjml-core";
import React from "react";

interface ConvertedHtml {
  html: string;
  json?: MJMLJsonObject;
  errors?: MJMLParseError[];
}

export function render(
  email: React.ReactElement,
  options: MJMLParsingOptions = {}
): ConvertedHtml {
  const defaults: MJMLParsingOptions = {
    keepComments: false,
    beautify: false,
    validationLevel: "strict",
  };

  const parseResults: ConvertedHtml = mjml2html(renderToMjml(email), {
    ...defaults,
    ...options,
  });

  return parseResults;
}
Bertg commented 1 year ago

@IanEdington I was suggesting that as a v4 suggestion.

emmclaughlin commented 1 year ago

Agree with the above discussion around removing mjml dependency in v4. That is, the main goal of this project is to be able to get the benefits mjml provides for writing emails while still developing in a react environment. With that in mind, our focus should stick to going from react -> mjml. Going from mjml -> html can be left up to the developer to implement in a way that suits their needs.

IanEdington commented 1 year ago

@Bertg were you able to get storybook working? Would you be willing to share what you had to do to get it working?

Bertg commented 1 year ago

@IanEdington Yeah, absolutely. I'm just kinda swamped with work the last 2 weeks, so progress is slow to "finish" it.

@emmclaughlin I understand your argument, but I'm looking at this from an other perspective. As technical team lead I'm looking at the building blocks that I need to send out emails, and make those choices ideal for my team.

So in case of sending emails, I choose nodemailer to send the emails, that's pretty clear. And I'm looking for a tool so that my team with React experience can design and implement the emails. I want as little hassle as possible to make this work.

If this project would make me choose "how" the react mjml is converted to the actual HTML, I would be far less inclined to use it. To me this would be similar to react saying the entire react-dom responsibility lays with the developer.

Sure the developer should be able to "change" the mjml -> html implementation, but this project needs a good default react -> html option built in.

To be even a bit more extreme, I would say that this project should do react -> [html, title] to be exact. Mjml allows you to define a mjml-title attribute, and right now, it's a bit of a hassle to extract that from the HTML or the JSON structure.

Bertg commented 1 year ago

@IanEdington Ok, I pushed my working version here: https://github.com/Bertg/mjml-react-storybook-example

Any feedback on the code or the readme would be greatly appreciated :)

IanEdington commented 1 year ago

cool thanks