mjml
mjml copied to clipboard
feat: add support for ESM module transpilation
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.
That look really great ! @kmcb777 can you take a look to get this merged ?
We could also add conditional exports here so we can support native ESM for node.js as well?
@DRoet good idea! I will add the fields in the package.json
files! 💪
@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!
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 should be solved
This should be good for 4.10 then ! i'm waiting for the CI and then merging this for 4.10
@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.
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
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!!!
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 did a small fixup
commit to fix the ESLint issues; should be alright.
@TrySound This change might increase the build size ? Is there any solution to that if so ?
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 ?
Yes, it will inrease package size. Though this can be avoided by dropping node 10 and cjs support.
Any idea when this can be resolved and merge, im current having issues using mjml with ES6
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 ?
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:
Some news from that?