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

MJML5.0 Replace html-minifier and js-beautify #2858

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

iRyusa
Copy link
Member

@iRyusa iRyusa commented Apr 25, 2024

Edit: Available on MJML on v5 for now on experimental tag

@iRyusa iRyusa added the MJML 5 label Apr 25, 2024
@marvin-wtt
Copy link

I currently get the error nullTypeError: Cannot destructure property 'children' of 'element' as it is undefined. When compiling with CLI

@iRyusa
Copy link
Member Author

iRyusa commented May 4, 2024 via email

@marvin-wtt
Copy link

Make sure to have no global install, clean your node modules + lockfile

Just tried it with a fresh project and only the Black Friday template as test. Still fails.
It also fails in CI, so this can't be a global install issue.

@iRyusa
Copy link
Member Author

iRyusa commented May 14, 2024

@marvin-wtt found out the issue thanks for reporting check the latest version (alpha.2) should be good now 👍

@thewilkybarkid
Copy link

thewilkybarkid commented Jul 16, 2024

How experimental is this now? I see there have been alpha releases. Looks like CI isn't working as support for Node 16 needs to be dropped (cssnano doesn't support it as it's unmaintained).

@iRyusa iRyusa force-pushed the fix/replace-html-minifier branch from 54ebade to 097a2bd Compare July 30, 2024 09:09
@iRyusa
Copy link
Member Author

iRyusa commented Jul 30, 2024

I don't think we'll find some alternative to those lib now. I still need some work tho to bring back the CI to a green state

@AaronMoat
Copy link

@iRyusa for what it's worth, the CI commands locally seem happy enough if packages/mjml/test/html-attributes.test.js is updated to await mjml instead of just mjml; something similar to test.js seems to work:

async function run() {
  const { html } = await mjml(input)

  ... existing ... 
}

run();

@iRyusa
Copy link
Member Author

iRyusa commented Aug 3, 2024 via email

@AaronMoat
Copy link

Is there any other support that you might want to get this over the line from experimental to shipped @iRyusa? 😄

@iRyusa iRyusa changed the title [EXPERIMENTAL] Replace html-minifier and js-beautify MJML5.0 Replace html-minifier and js-beautify Oct 4, 2024
@iRyusa
Copy link
Member Author

iRyusa commented Oct 4, 2024

Alpha 6 might be the last alpha version. Then I'll push it to the "live".

@robkorv
Copy link

robkorv commented Oct 18, 2024

@iRyusa thank you for this change. It's going to be deployed to our production on Monday 🤞🏽

@iRyusa
Copy link
Member Author

iRyusa commented Oct 18, 2024

Feel free to reach me on slack if you find any issues 👍

@jwaterworth
Copy link

@robkorv How was your deployment? Anything we need to be aware of in using this release?

Thanks @iRyusa for making this available! 🙏

@robkorv
Copy link

robkorv commented Nov 18, 2024

@robkorv How was your deployment? Anything we need to be aware of in using this release?

Without any problems. About 415.358 mail build with "5.0.0-alpha.6" since the deploy. The only thing that is changed is that mjml2html is now asynchronous. Everything else is still the same.

I use mjml in a microservice build with hapi. I only needed this change in a hapi route to make it work.

@@ -1,37 +1,40 @@
 "use strict";
 
 const Boom = require("@hapi/boom");
 const Mjml = require("mjml");
 
 module.exports = [
   {
     method: ["get", "post"],
     path: "/mjml",
-    handler: (request, h) => {
+    handler: async (request, h) => {
       if (request.method === "post") {
         try {
-          const result = Mjml(request.payload.mjml, request.payload.options);
+          const result = await Mjml(
+            request.payload.mjml,
+            request.payload.options
+          );
 
           if (request.payload.return_html) {
             const formattedMessages = result.errors
               .filter((x) => x.formattedMessage)
               .map((x) => x.formattedMessage);
             if (formattedMessages.length) {
               request.logger.warn(formattedMessages.join("\n"));
             }
 
             return result.html;
           }
 
           return result;
         } catch (err) {
           const boom = new Boom.Boom(err.toString());
           boom.reformat(true);
           return boom;
         }
       } else {
         return "post mjml and I will return html";
       }
     },
   },
 ];

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

Successfully merging this pull request may close these issues.

High Severity Vulnerability in html-minifier
6 participants