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

esbuild for v11 #4109

Closed
wants to merge 49 commits into from
Closed

esbuild for v11 #4109

wants to merge 49 commits into from

Conversation

sidharthv96
Copy link
Member

@sidharthv96 sidharthv96 commented Feb 19, 2023

📑 Summary

Replace vite with esbuild as we are ESM only now.

📏 Design Decisions

🚀 speed.

# Full Build
ESBuild -> nr build  11.38s user 1.09s system 226% cpu 5.497 total
Vite    -> nr build  31.84s user 3.15s system 170% cpu 20.535 total
# JS only build
ESBuild -> nr build:esbuild  4.49s user 0.68s system 241% cpu 2.143 total
Vite    -> nr build:vite  24.21s user 2.57s system 163% cpu 16.402 total

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 📓 have added documentation (if appropriate)
  • 🔖 targeted develop branch

* release/10.0.0: (1038 commits)
  Fix docs
  fix Server
  Fix lint
  Remove Readme
  Fix E2E Tests
  Fix tests
  feat: Break render and parse types
  chore: Remove all non async render/parse/init
  Remove CJS builds from docs
  chore: Remove cjs from build
  RC version
  Revert #4034
  Revert #4034
  fix: Vite, D3, Vitest Types
  fix(api): tree shaking package.json import
  Add highlight tag info in contributing.md
  chore(deps): update dependency cypress to v12
  docs: fix links
  Skip precommit hooks on CI
  Fix release-publish
  ...
* release/10.0.0:
  fix(#3406, #3394): Remove init & initThrowsErrors
  chore: Rename lazy loaded diagram definitions
  Skip flowchart-elk failing test
* release/10.0.0:
  Cleanup
  Update docs
@sidharthv96 sidharthv96 changed the title Sidv/esbuild v10 esbuild for v10 Feb 19, 2023
@sidharthv96 sidharthv96 linked an issue Feb 19, 2023 that may be closed by this pull request
@sidharthv96 sidharthv96 self-assigned this Feb 19, 2023
Base automatically changed from release/10.0.0 to master February 20, 2023 12:28
@sidharthv96 sidharthv96 changed the base branch from master to release/10.0.0 February 20, 2023 12:47
@sidharthv96 sidharthv96 changed the base branch from release/10.0.0 to develop February 24, 2023 07:52
* develop: (57 commits)
  fix Lint
  Update CHANGELOG.md
  Update CHANGELOG.md
  fix: fix exports
  Fix readme link
  Regenerate mermaid docs
  Add deepdwn to cspell
  Add Deepdwn to native integrations list
  docs: Fix changelog
  docs: v10 breaking changes
  Remove `null` from diagrams before render
  fix docs diagram
  Updated version
  Minor cleanup to trigger build.
  Fix spellings
  Wrap option working in test case
  Fix typos
  Minor cleanup
  Removed the deprecated use of mindmap in Demo
  Minor cleanup
  ...
@sidharthv96
Copy link
Member Author

sidharthv96 commented Aug 12, 2023

Released https://www.npmjs.com/package/mermaid/v/11.0.0-alpha.1 with IIFE build.

When using <script src="CDN/mermaid.min.js">, mermaid.<function> will have to be replaced with mermaid.default.<function>. But, all diagrams will be rendered onLoad as usual.

I have added a console error suggesting the change if that error is detected

sidharthv96 and others added 4 commits August 12, 2023 11:14
* next:
  Update assignWithDepth.ts
  Version update and adjusted error diagram
  Fix for broker error diagram related #4178
  Adding new flowchart tests related to issue #2139
  Fix for interim issue with classes in state diagrams
  redeclare `config` parameter add default value for each variable
  convert `assignWithDepth` to TS
  #2139 Applying user defined classes properly when calculating shape width
  chore: Ignore localhost
  Update packages/mermaid/src/docs/community/development.md
  Update docs/community/development.md
  docs: Add development example page.
  docs: Correct detectType filename
  chore: Minor cleanups
  chore: remove comment
  chore: Remove comments, cleanup
  fix: unitTests after tripleParsing removal
  fix: Remove triple parsing of diagrams
@sidharthv96 sidharthv96 added this to the v11 milestone Aug 12, 2023
@sidharthv96
Copy link
Member Author

TODO in this PR

  • Verify E2E coverage

Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to ESBuild and using IIFE seems to fine to me, but I'm pretty against breaking the usage <script/> by changing globalThis.mermaid to globalThis.mermaid.default. But I think this should be fixable (even if we have to do it manually), see my comment below:


Also, I've notice that this PR has a lot of git merge commits, and the first new commit on this PR (9763c03) uses 8e3f986 as a parent, which is a commit from September 2022, almost a year ago.

Does it make sense to use git rebase to clean up the commit history?

  • Benefits: it will make future merge conflicts much easier to handle (pretty important if we're aiming for the next/ branch instead of develop).
  • Downsides: git rebase can be really hard to learn, unless you're already a git CLI expert.

One final thing, depending on when you want v11 to be released, we could also do something like:

  • v11 breaking changes
    • Switch Vite UMD output of mermaid.min.js to Vite IIFE output of mermaid.min.js (still using Vite, so it would be a very small code change, maybe just one line of code)
  • v11.0.1 change
    • Switch from Vite to ESBuild (no breaking changes since we already did the UMD -> IIFE switch in a previous release)
      • This would be able to go directly to the develop branch, so less risk of merge conflcits.

It would mean less risk of merge conflicts, and it would make the v11 BREAKING CHANGES easier to view. The only downside is then @Yokozuna59 would need to wait a few more weeks for ESBuild.

.build/common.ts Show resolved Hide resolved
packages/mermaid/src/mermaid.ts Outdated Show resolved Hide resolved
sidharthv96 and others added 3 commits August 12, 2023 20:00
Co-authored-by: Alois Klink <alois@aloisklink.com>
Co-authored-by: Alois Klink <alois@aloisklink.com>
@Yokozuna59
Copy link
Member

The only downside is then @Yokozuna59 would need to wait a few more weeks for ESBuild.

I won't be able to work much on it after a few weeks since the summer holiday ends 😬. That's why I'd recommend supporting more diagrams and grammars; then integrating would be easier since it's just inserting values from AST (without converting other diagram files, just DB).

@sidharthv96
Copy link
Member Author

sidharthv96 commented Aug 12, 2023

@Yokozuna59 now that the POC works, you can carry on with rest of the migration in that branch (After the info and pie PRs are done as we discussed).
We can make separate PRs to integrate individual diagrams in a phased out manner, while porting any JISON changes meanwhile.

package.json Outdated Show resolved Hide resolved
@sidharthv96
Copy link
Member Author

Closing in favour of #4729

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

Successfully merging this pull request may close these issues.

min.js is not minified
3 participants