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

Switch to ESM only for v10? #3590

Closed
Tracked by #4108 ...
sidharthv96 opened this issue Oct 6, 2022 · 7 comments · Fixed by #4108
Closed
Tracked by #4108 ...

Switch to ESM only for v10? #3590

sidharthv96 opened this issue Oct 6, 2022 · 7 comments · Fixed by #4108

Comments

@sidharthv96
Copy link
Member

The current build setup is complicated (and slow).

Pros:

  • Simpler builds
  • Faster builds with esbuild
  • Tree shaking?

Cons:

  • Different import script in browsers
  • Loses support for legacy browsers? (@knsv This might be an issue with using module: ES2022 in lazy load also)
  • Downstream projects might have to handle small migration. But not sure if anything will be broken as require has already been broken due to D3 dependency as @aloisklink mentioned here

Related:

@github-actions github-actions bot added the Status: Triage Needs to be verified, categorized, etc label Oct 6, 2022
@sidharthv96 sidharthv96 added Area: Development Type: Question and removed Status: Triage Needs to be verified, categorized, etc labels Oct 6, 2022
@aloisklink
Copy link
Member

Another benefit of moving to ESM only, it will make npm install mermaid much smaller (I wonder how much CO2 it will save if we cut 15 MiB over hundreds of thousands of downloads per week!)

Ideally we should only have:

  • mermaid.core.mjs (maybe we should also minifiy this)
  • mermaid.esm.min.mjs (although maybe we should rename it to mermaid.production.mjs, since it should only be used by people using CDN links like https://unpkg.com/)

@knsv
Copy link
Collaborator

knsv commented Oct 7, 2022

I find this very tempting but also find it is a hard to understand the consequences for downstream projects.

@sidharthv96
Copy link
Member Author

Gitlab already uses our ESM build. https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/lib/mermaid.js
I'm guessing GitHub and other major players will be doing something similar.

Are there any important projects that we could check with?

@aloisklink
Copy link
Member

aloisklink commented Oct 7, 2022

I find this very tempting but also find it is a hard to understand the consequences for downstream projects.

Essentially, if a downstream project is using:

  • const mermaid = require("mermaid");
    This has been broken for a while (since sometime in v8), see Requiring from CommonJS is broken #2559.
    Edit: it looks like some bundlers (e.g. TypeScript) sometimes do still support this. It's a bit wishy-washy when it works and when it doesn't work.
  • import mermaid from "mermaid";: No change
  • If using the pre-bundled version of Mermaid, the following UMD/IIFE syntax will no longer be supported:
    <script src="https://cdn.jsdelivr.net/npm/mermaid/dist/mermaid.min.js"></script>
    <script>
      mermaid.initialize({startOnLoad:true});
    </script>
    • Instead, the ESM syntax would be:
    <script type="module">
      import mermaid from "https://cdn.jsdelivr.net/npm..."
      mermaid.initialize({startOnLoad:true});
    </script>
    This ESM syntax is supported in every modern browser for quite a while: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules
    However, it's not supported on Internet Explorer or Opera Mini, but it might not be worth supporting these old browsers: https://caniuse.com/es6-module
    If a downstream project really needs to support IE/Opera Mini, they can bundle Mermaid themselves to an IIFE build.

Potential issue: If websites have used <script src="https://cdn.jsdelivr.net/npm/mermaid/dist/mermaid.min.js"/> (which is what the docs recommended: https://github.com/mermaid-js/mermaid/blob/develop/docs/README.md#deploying-mermaid), this means that if we publish a v10, since the URL always downloads the latest version, this might break people's websites. For jsdelivr, we can fix this by renaming the file to a different name:

image

Edit: Because of that, I think we should keep the mermaid.min.js UMD/IIFE bundle, but we can remove the ability to require("mermaid");.

@sidharthv96
Copy link
Member Author

We should fix those docs asap. I'll change it now.

@knsv
Copy link
Collaborator

knsv commented Oct 7, 2022

In principle we are not running in node but in the browser which is a point for esm. What I find hard about this is to figure out how this will affect the downstreams projects

@Pezmc
Copy link

Pezmc commented Feb 22, 2023

As discussed above, I can confirm this has broken packages that include Mermaid via unpkg: i.e. https://unpkg.com/mermaid/dist/mermaid.min.js

JS deliver is indeed serving a cached version instead of the 404.

Swapping to https://unpkg.com/mermaid@10/dist/mermaid.esm.min.mjs resolves the issue
(or use https://unpkg.com/mermaid@9/dist/mermaid.min.js if you can't switch to ESM).

Edited by @aloisklink to add @9/@10 version specifiers to unpkg URLs, as there's no guarantee that a future Mermaid version 11 will not change these files)

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

Successfully merging a pull request may close this issue.

4 participants