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

feat: add support for ESM module transpilation #2273

Closed
wants to merge 2 commits into from

Conversation

bjufre
Copy link

@bjufre bjufre commented May 28, 2021

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 bjufre force-pushed the feat/esm-target-support branch from 3b22d20 to 12f8f19 Compare May 28, 2021 13:09
@iRyusa
Copy link
Member

iRyusa commented May 28, 2021

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

@bjufre bjufre force-pushed the feat/esm-target-support branch from 12f8f19 to 5f38224 Compare May 28, 2021 13:18
@DRoet
Copy link
Contributor

DRoet commented Jun 2, 2021

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

@bjufre
Copy link
Author

bjufre commented Jun 2, 2021

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

@bjufre
Copy link
Author

bjufre commented Jun 2, 2021

@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!

@iRyusa
Copy link
Member

iRyusa commented Jun 10, 2021

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

@bjufre bjufre force-pushed the feat/esm-target-support branch from 400f5bb to ce4ce53 Compare June 11, 2021 05:47
@bjufre
Copy link
Author

bjufre commented Jun 11, 2021

@iRyusa should be solved

@iRyusa
Copy link
Member

iRyusa commented Jun 11, 2021

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

@bjufre
Copy link
Author

bjufre commented Jun 11, 2021

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

@iRyusa
Copy link
Member

iRyusa commented Jun 11, 2021

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

@bjufre
Copy link
Author

bjufre commented Jun 11, 2021

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!!!

@iRyusa
Copy link
Member

iRyusa commented Jun 11, 2021

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 🤔

@bjufre
Copy link
Author

bjufre commented Jun 11, 2021

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

@iRyusa
Copy link
Member

iRyusa commented Jun 17, 2021

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

@kmcb777
Copy link
Collaborator

kmcb777 commented Jun 17, 2021

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 ?

@TrySound
Copy link
Contributor

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

@BadMask121
Copy link

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

@iRyusa
Copy link
Member

iRyusa commented Jun 18, 2021

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

iRyusa commented Jun 29, 2021

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:

@kmcb777 kmcb777 added the MJML 5 label Jan 13, 2022
@multivoltage
Copy link

Some news from that?

@iRyusa
Copy link
Member

iRyusa commented Oct 5, 2024

Closing for now I wonder if this get resolved by itself with typescript migration #2908

@iRyusa iRyusa closed this Oct 5, 2024
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.

7 participants