mjml icon indicating copy to clipboard operation
mjml copied to clipboard

feat: add support for ESM module transpilation

Open bjufre opened this issue 3 years ago • 19 comments

With the rise of dev/build tools like Vite or Esbuild, the ecosystem seems to be going in the direction of ES modules as they're currently supported in all major browsers. Nonetheless, these tools, depending on the library imports being esm friendly. And even though, Vite, for example, offers the optimizeDeps option in order to get around the issue of some imports not using esm, it loses some of the advantages of tree shaking and or it's not possible to use the libraries as plugin for the project config itself; e.g: creating a custom local compilation/preprocess for compiling <template lang="mjml" /> (Sveltekit).

This PR simply adds the required changes in the package.json on all the packages that do not involve the browser (mjml-browser) and/or the cli (mjml-cli & mjml-migrate), as I in that case wasn't completely sure what should be the correct direction, but they are libraries that might not be targeted for those dev/build tools from the start. The changes also affect the main babel.config.js so that we can target the correct module system based on whether we're building for cjs or esm.

Hope this can be discussed and improved further if need be.

bjufre avatar May 28 '21 04:05 bjufre

That look really great ! @kmcb777 can you take a look to get this merged ?

iRyusa avatar May 28 '21 13:05 iRyusa

We could also add conditional exports here so we can support native ESM for node.js as well?

DRoet avatar Jun 02 '21 05:06 DRoet

@DRoet good idea! I will add the fields in the package.json files! 💪

bjufre avatar Jun 02 '21 06:06 bjufre

@DRoet added a small fixup commit to add the fields in the different packages package.json. Give it a look and see what you think!

bjufre avatar Jun 02 '21 15:06 bjufre

I'm not really experienced enough with ESM modules but this looks good to me to merge for next release ? Can you just resolve yarn lock conflict please

iRyusa avatar Jun 10 '21 20:06 iRyusa

@iRyusa should be solved

bjufre avatar Jun 11 '21 05:06 bjufre

This should be good for 4.10 then ! i'm waiting for the CI and then merging this for 4.10

iRyusa avatar Jun 11 '21 09:06 iRyusa

@iRyusa Seems that there was some kind of problem with the CI? 🤔 Although from a first glance I don't see how the changes might affect that.

bjufre avatar Jun 11 '21 09:06 bjufre

Well I guess we might need to refactor those absolute import then D: https://github.com/mjmlio/mjml/blob/master/packages/mjml-carousel/src/Carousel.js#L5

iRyusa avatar Jun 11 '21 09:06 iRyusa

Well I guess we might need to refactor those absolute import then D: https://github.com/mjmlio/mjml/blob/master/packages/mjml-carousel/src/Carousel.js#L5

@iRyusa I will take a look in about 30' I have to take care of something for my work and I will get back to this once finished!

But it's weird, I was able to build it locally before without a problem, but I will make sure, sorry for the back and forth man!!!

bjufre avatar Jun 11 '21 09:06 bjufre

No worry you really did a good job already ! 👍

I'll try to take a look too, I wonder we can just remove those /lib to see if it resolve it properly on its own now 🤔

iRyusa avatar Jun 11 '21 10:06 iRyusa

@iRyusa did a small fixup commit to fix the ESLint issues; should be alright.

bjufre avatar Jun 11 '21 11:06 bjufre

@TrySound This change might increase the build size ? Is there any solution to that if so ?

iRyusa avatar Jun 17 '21 13:06 iRyusa

tbh i'm not especially experienced with esm modules either, i was wondering if there could be consequences of having two builds for each package ? i noticed that you replaced some of the imports mjml-core/lib with sometimes mjml-core/lib/cjs and sometimes mjml-core/lib/esm , is there a particular reason for this ?

kmcb777 avatar Jun 17 '21 13:06 kmcb777

Yes, it will inrease package size. Though this can be avoided by dropping node 10 and cjs support.

TrySound avatar Jun 17 '21 15:06 TrySound

Any idea when this can be resolved and merge, im current having issues using mjml with ES6

BadMask121 avatar Jun 18 '21 04:06 BadMask121

Well maybe it's time for us to drop node 10 as it's no longer supported anyway. If it still works on node 12 I guess we can drop cjs ?

iRyusa avatar Jun 18 '21 10:06 iRyusa

So can you remove node 10 from https://github.com/mjmlio/mjml/blob/master/.github/workflows/mjml-workflow.yml#L8 and CJS support ?

And so sorry for thoses conflict I think we did some update on 4.10.1 D:

iRyusa avatar Jun 29 '21 22:06 iRyusa

Some news from that?

multivoltage avatar Dec 25 '23 19:12 multivoltage