Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
IanEdington opened this issue Feb 21, 2023 · 9 comments
Open
Milestone

Comments

@IanEdington
Copy link
Contributor

IanEdington commented Feb 21, 2023

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:

  • mj-include tags are unavailable and will be ignored.
  • features involving the .mjmlconfig file are unavailable, which means no custom components.

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 #89 (comment)

related issues:

@IanEdington IanEdington added this to the v4 milestone Feb 21, 2023
@Bertg
Copy link

Bertg commented Feb 23, 2023

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
Copy link
Contributor Author

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
Copy link

Bertg commented Feb 23, 2023

@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
Copy link
Contributor Author

IanEdington commented Feb 23, 2023

Some unrelated thoughts:

  • lots of tickets are due to this specific issue
  • switching to mjml-browser will make it very difficult for users to migrate to the package
  • There are SO many ways a render function can be built (we have 3-4 internally)
    • implementers should almost always customize the render
    • another option: we could provide 10 renderers or +++parameters on the render function
  • The basic render function is SO simple
  • We have very limited bandwidth to work on this so reducing complexity of the package to deliver the 80% of the value with 20% of the effort is one of our primary goals.

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 added a commit that referenced this issue Feb 24, 2023
IanEdington added a commit that referenced this issue Feb 24, 2023
IanEdington added a commit that referenced this issue Feb 24, 2023
@IanEdington
Copy link
Contributor Author

IanEdington commented Mar 6, 2023

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

@IanEdington IanEdington changed the title Could this project depend on mjml-browser instead of mjml? Make this project agnostic to runtime environment (node/browser) Mar 12, 2023
@pachuka
Copy link

pachuka commented Mar 11, 2024

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 wix-incubator/mjml-react#84 (comment) 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 wix-incubator/mjml-react#75.

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

@emmclaughlin
Copy link
Collaborator

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
Copy link

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

@emmclaughlin
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants