standardize diagram definitions

[Yokozuna59]

Most of these rules were discussed in this PR #4486, but it became kind of messy with commits and reviews, so we're going to summarize them here (and update them continuously):

• *.js files aren't allowed; use *.ts instead.

• try not to use // @ts-* comments, but if you have, please add a description showing the problem. i.e.:

  // @ts-ignore - jison doesn't export types
  import jison from './parser/diagram.jison';
  
  // @ts-ignore - couldn't cast Document type into HTML
  const html: HTML = docuement;

• try not to use default imports/exports, use named one.

  // don't do this
  import Variable from './file.js';
  import * as file from './file.js';
  export default Variable;
  
  // do this
  import { Variable } from './file.js';
  export { Variable };

• use types instead of any.

---

• Standardize diagram words #4542

General Diagram Definition

// diagarm = Name of diagram, e.i., pie, flowchart, etc.

packages/mermaid/src/diagrams/diagram/
├── parser/
│   └── diagram.jison
├── diagram.spec.ts
├── diagramDb.ts
├── diagramDetector.ts
├── diagramDiagram.ts
├── diagramRenderer.ts
├── diagramStyles.ts
└── diagramTypes.ts

• parser/diagram.jison: contains parser and grammar of the diagram.
• diagramDb.ts:
  • DEFAULT_DIAGRAM_CONFIG.
  • DEFAULT_DIAGRAM_DB.
  • fields with their functions, i.e., getters and setters.
  • title and accessibilities getters and setter.
  • custom clear function (or use common one).
  • config getter.
  • parseDirective.
• diagramDiagram.ts: contains one variable that exports the following:
  • parser.
  • db.
  • renderer.
  • styles (if needed).
• diagramDetector.ts:
  • id (diagram keyword).
  • detector.
  • loader.
  • diagram (export the above variables/functions).
• diagramRenderer.ts:
  • draw function.
• diagramStyles.ts:
  • getStyles function (contains css styles for the diagram).
• diagramTypes.ts:
  • DiagramDiagramConfig.
  • DiagramFields.
  • DiagramStyleOptions.
  • DiagramDB.
  • other types if needed.

[Yokozuna59]

I've more thoughts:

Files

1. Add diagramTheme.ts: Mermaid uses lazy loading for rendering diagrams, it doesn't make since to set and use other diagram themes when the user trying to render one diagram (unless you're planning to add multiple diagram rendering in one code). So by creating this file, it would be helpful for getting and setting values with ts code instead of mermaid, and resolve variables name duplication.

Structure

1. Make the diagramRenderer uses it getCofnig instead of global one.
2. Make the returned values from diagram getConfig return actual values, not possible underfed. The undefined interface is for overriding default config, not when getting it.
3. Add setConfig and reset for diagram itself, and the parent setConfig and reset should uses it.
4. Separate the returned the type valued value from getConfig and setConfig.

---

Please let me know if you approve the below and above suggestion so I can the original issue, or you could change it right away.

[Yokozuna59]

And shouldn't better to move types in config.type.ts into each diagramTypes.ts?

[sidharthv96]

All the suggestions look great to me.

[Yokozuna59]

All the suggestions look great to me.

Great! Could you add labels to the issue? i.e, Discussion.

And do you have something in your mind on behave of separating setConfig parameter type and getConfig return type? Because it shows as optional field whereas it's for sure has default value.

It's not clean to do the the following:

const width = config?.pie?.useWidth;

[sidharthv96]

Yes, getConfig should not have optionals. But we have to make sure to do it properly (without forced type assertion if possible), so that we get compile time errors if there is no default value for a config.

[Yokozuna59]

Yes, getConfig should not have optionals. But we have to make sure to do it properly (without forced type assertion if possible), so that we get compile time errors if there is no default value for a config.

One possible approach is using Required type, something like this:

export const DEFAULT_PIE_CONFIG: Required<PieDiagramConfig> = {
   useMaxWidth: true,
   useWidth: 1200,
   textPosition: 0.75,
 } as const;

By doing this, it well force to set all values of the type, even optional one. Theoretically, it will always shows that it defined. And we could use RequiredDeep for object types from type-fest

What do you think? I'll update the top message with suggested stuff in here https://github.com/mermaid-js/mermaid/issues/4499#issuecomment-1595189472 with this if all of them have been approved.

[Yokozuna59]

It would be great if we standardized diagram keywords, because some diagrams just have the actual keyword:

• pie
• timeline
• gantt
• mindmap ...etc.

And some have additional word:

• quadrantChart
• stateDiagram
• erDiagram
• sequenceDiagram ...etc.

---

I think this addition wouldn't really help; it's just making the keyword longer.

[Yokozuna59]

And maybe add renderer directory?

packages/mermaid/src/diagrams/diagram/
├── ...
├── renderer/
│   ├── note.ts
│   ├── edge.ts
│   └── ...
└── diagramRenderer.ts # export functions from renderer dir

The number of files related to rendering is kinda large in number and size, specially flowchart, so it might be better to create a folder that contains all of those stuff. So that the diagram dir would be cleaner.

What do you think?

[sidharthv96]

I'm aware that the root message is kind of outdated, but I haven't been receiving any feedback on the suggested ideas. Could I assume that they have been approved with those likes? @sidharthv96 https://github.com/mermaid-js/mermaid/issues/4542#issuecomment-1609376984

Please consider the comments marked with 👍 as me agreeing personally, not as an official approval of any sorts. We might have to make edits when others review the changes once they are inside a PR. 😄

[nirname]

Did we mention integration specs here?

[Yokozuna59]

Did we mention integration specs here?

It might be better to mention in the script that generate new diagram, because I think this is related to definition itself, not all files related to diagram.

[nirname]

I have noticed that the name of the file contains diagram name as well (pieRenderer.ts). It may be even better and informative, but it is also excessive a bit.

Share your thoughts on (informative, excessive)

packages/mermaid/src/diagrams/diagram/
├── parser/
│   └── diagram.jison
├── diagram.spec.ts
├── diagramDb.ts
├── diagramDetector.ts
├── diagramDiagram.ts
├── diagramRenderer.ts
├── diagramStyles.ts
└── diagramTypes.ts

versus (less informative, but very string and uniform)

packages/mermaid/src/diagrams/diagram/
├── parser.jison
├── tests.spec.ts
├── DB.ts
├── detector.ts
├── diagram.ts
├── renderer.ts
├── styles.ts
└── types.ts

[Yokozuna59]

IMO, the full form (informative) form is better because we can refer to diagram files directly rather than in the short form, and here are some reasons why:   In reviewing, GitHub shows and sorts files by file name, so almost all related files (to the actual diagram) will be grouped together. Files are stored inside a 5-6 folder hierarchy, so the actual folder (diagram name) most likely won't be visible, so we won't be able to know in what diagram what file has been modified right away. If we changed the spec folder into tests, we can't filter test cases easily.

But I agree with moving jison into diagram folder.

[Yokozuna59]

It might be better to add new file called diagramDefaults.ts, this file would contain the default values for DBs, styles, themes, configs ...etc. rather than adding them to diagramDb.ts.

[nirname]

Didn't get your idea about defaults folder. There are too many things that can be called "default", so I'll try to sort things through:

• if we are talking about adding new diagram, then there should be a diagramFactory that does the trick; one of the good examples is rails generators. Factory would generate default structure. I am not sure where specs should reside, we have integration tests in cypress folder and unit vitests scattered across the project.
• there are not so many things in diagramDB itself to be separated as a file with defaults; variables that must be initialized should stay in clear() function, or even better in a constructor, if DB wrapped as a class, thus clear probably equals to assigning new database. So default values for DB goes to constructor
• there is a globally defined defaultConfig, and there is https://github.com/mermaid-js/mermaid/pull/4112 that structures it which we probably should wait for, but I think that defaultConfig must be a file in diagram folder, so as to avoid extending this global config, which should be automatic composition of diagram-specific configs

[Yokozuna59]

Didn't get your idea about defaults folder.

Just a typo :), my intention was a file. A file within diagram folder.

[nirname]

Fully agree with that

[sidharthv96]

I am aligning closer to the naming convention suggested by @nirname. Our editors are smart enough to detect pie/db if we type piedb to open a file. GitHub shows the full path in reviews. So I don't really see why we should keep the diagram/diagramDB.ts convention instead of the cleaner diagram/DB.ts.

[Yokozuna59]

Then should we rename files in #4727 for parser package?

[sidharthv96]

I don't think that's necessary right now. We can discuss and decide later and do a single PR to implement the decision.