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

splitDiagrams #4110

Merged
merged 35 commits into from
Feb 20, 2023
Merged

splitDiagrams #4110

merged 35 commits into from
Feb 20, 2023

Conversation

sidharthv96
Copy link
Member

📑 Summary

Splits all diagrams into lazy loaded modules.

📏 Design Decisions

Mermaid will only load whichever diagram is being used.

📋 Tasks

Make sure you

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

* develop: (233 commits)
  style(docs): use `github-dark` hightlight theme
  refactor(docs): use default vitepress highlighter
  fix: Move redirection to router
  ci(renovate): disable pinning dependencies
  Revert "chore(deps): pin dependencies"
  change shiki getHighlighter import
  create separate spec for stateRenderer-v2
  diagramStates should not be global; pass it into functions; minor comment fixes
  diagramClasses no longer needs to be cached; mermaidAPI no longer calls it repeatedly
  (minor) import expectTypeOf in spec
  (minor) fix JSdoc tag
  + spec stateRenderer-v2.js getClasses() to verify it returns a {}
  (minor) fix JSdoc types in comments
  (minor) add comments, remove duplicated line
  chore: Add master to link checker
  chore: Add docs to redirect.ts
  stateDB classes must be a {} not []
  feat: Redirect old documentation links.
  add stateDiagram-v2 to list of graphs with classDefs
  fix(docs): ClassDiagram table
  ...
* sidv/sizeCheck:
  feat: Add size inspection plugin
* sidv/viz:
  feat: Add package visualization
  Ignore stats.html
  feat: Add bundle visualization
lodash with specific imports use lesser space than lodash-es
* develop: (79 commits)
  Minor change
  feat: Add @include support to docs
  feat: Add @include example to docs
  feat: Add @include support to docs
  cleanup
  fix lines
  fix Async rendering
  Revert "sync"
  chore(deps): update pnpm to v7.17.1
  chore(deps): remove dependency on `graphlib`
  test(e2e): make gitgraph snapshots consistent
  chore: Fix lint
  test: Update vitest
  Add official vim plugin to list in integrations
  chore: Cleanup package.json
  chore: Cleanup package.json
  chore: Cleanup package.json
  fix lock
  Docs
  Fix: array concat
  ...
* release/10.0.0: (333 commits)
  10.0.0-rc.3
  Export more types
  no side effects
  10.0.0-rc.2
  skip failing elk test
  Cleanup
  Update docs
  fix(#3406, #3394): Remove init & initThrowsErrors
  chore: Rename lazy loaded diagram definitions
  Skip flowchart-elk failing test
  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
  ...
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:46
@pbrolin47 pbrolin47 merged commit 786023f into release/10.0.0 Feb 20, 2023
registerDiagram(id, diagram, detector);
} catch (err) {
// Remove failed diagram from detectors
log.error(`Failed to load external diagram with key ${key}. Remozing from detectors.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typo. Change 'Remozing from detectors' to 'Removing from detectors'

export const c4Detector: DiagramDetector = (txt) => {
const id = 'c4';

const detector = (txt: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the majority of other diagram detectors have a declaration "const detector: DiagramDetector = ...",
but in this file the detector type is not present. It's only declared as "const detector = ..."

import erRenderer from './erRenderer';
import erStyles from './styles';

export const diagram = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add type definition "DiagramDefinition" as "export const diagram: DiagramDefinition = {"

import { MermaidConfig } from '../../config.type';
import { setConfig } from '../../config';

export const diagram = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add type definition "DiagramDefinition" as "export const diagram: DiagramDefinition = {"

import flowStyles from './styles';
import { MermaidConfig } from '../../config.type';

export const diagram = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add type definition "DiagramDefinition" as "export const diagram: DiagramDefinition = {"

Copy link
Contributor

@pbrolin47 pbrolin47 left a comment

Choose a reason for hiding this comment

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

Reviewed.

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.

Looks good to me!

Two quick questions:

  • Are we still publishing a giant mermaid.esm.min.mjs bundle that has everything in a single-file (i.e. without any lazy-loading)?
    I know there's still quite a few people that just copy a single file mermaid.min.js file to their projects (e.g. this is what joplin does, see https://github.com/laurent22/joplin/tree/dev/packages/renderer/assets/mermaid).
    (The mermaid-cli project used to do the same thing, but we recently had to switch to using vite to get mermaid-mindmaps working).
  • We should probably document this change as a BREAKING CHANGE in our release notes too. I think pretty much every bundler should support lazyLoading, but just in case, we should document it.
    Plus, this might also affect people using websites in offline mode, e.g. in PWA, as lazy-loading probably needs internet connection.

@Yokozuna59 Yokozuna59 deleted the sidv/splitDiagrams branch August 11, 2023 19:58
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.

3 participants