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

Diagram specific configuration with YAML (Sankey-beta missing diagram-specific configuration and parsing error) #4630

Closed
ilya-vv opened this issue Jul 11, 2023 · 21 comments · Fixed by #4750
Labels
Area: Development Status: Pending Is not to be executed as it currently is Type: Enhancement New feature or request

Comments

@ilya-vv
Copy link

ilya-vv commented Jul 11, 2023

Description

Sankey-beta diagram parser error when using diagram-specific configuration.

Console output:

Parse error on line 1:
%%{init: {'theme': '...
^
Expecting 'SANKEY', got 'NON_ESCAPED_TEXT'

Steps to reproduce

Add diagram-specific configuration:
%%{init: {'theme': 'default', 'sankey':{ 'width': 400}}}%%

Screenshots

With diagram-specific configuration:
image

Without:
image

Code Sample

%%{init: {'theme': 'default', 'sankey':{ 'width': 400}}}%%
sankey-beta
a 30,b 30,30
b 30,e 64,12
c 70,d 30,30
d 30,e 64,12
c 70,e 64,40
e 64,q 12,12

Setup

  • Mermaid version: develop [latest]
  • Browser and Version: Chrome 114.0.5735.198

Suggested Solutions

No response

Additional Context

No response

@ilya-vv ilya-vv added Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect labels Jul 11, 2023
@sidharthv96
Copy link
Member

For new diagrams, config should go in as yaml frontmatter.
I'm not sure if yaml config is currently supported in sankey, but %%{init: {'theme': 'default', 'sankey':{ 'width': 400}}}%% will not be supported.
If yaml config is not supported, the support should be added for complete config values, not just sankey.
It should be easier now that #4112 is merged.

@nirname
Copy link
Contributor

nirname commented Jul 11, 2023

Configuration for Sankey diagrams via yaml are not supported yet, please, use config for now.

@Yokozuna59
Copy link
Member

For new diagrams, config should go in as yaml frontmatter.

could please show an complex example?

@nirname
Copy link
Contributor

nirname commented Jul 11, 2023

Like this

---
title: Animal example
---
classDiagram

I consider that this yaml should not be a part of diagram grammar at all, this must be preprocessed separately

@sidharthv96
Copy link
Member

sidharthv96 commented Jul 11, 2023

That is processed separately, isn't part of grammar.
We just need to add support for the full config options to be read from this frontmatter (except the security focused ones).

@Yokozuna59
Copy link
Member

Yokozuna59 commented Jul 11, 2023

Like this

---
title: Animal example
---
classDiagram

I'm aware of the simple one, I was referring to the complex (objects) ones, for example configuring sankey should be something like this?

---
sankey:
  - width: 300
---
sankey

@nirname
Copy link
Contributor

nirname commented Jul 11, 2023

@Yokozuna59 It should be Yaml syntax. I know this is kinda weird answer, but it should represent JSON configuration 1 to 1.
I don't know should it contain the diagram key by itself or not. Perhaps it must contain only diagram-specific settings, thus the key is redundant. In case it contains some global options there are 2 possible approaches:

  • either reject the key and inherit all the global attributes
  • or keep the key as in your example.

I would stick to the first one
Maybe @sidharthv96 could explain it better.

Just in case: if I am not mistaken this --- between config and graph definition is a must.

@Yokozuna59
Copy link
Member

Each code should only be for one diagram. So, it doesn't make sense to configure values for other diagrams, i.e., the code is for pie, but the user configures values for gantt`:

%%{init: {"gantt": {"mirrorActor": true}}}%%
pie

In this case, the first approach would be better since all those values would be for the actual diagram.

But doing that, it would be hard to strictly limit possible keys, i.e., if pie1 is only for pie, we can't throw an error since it's a supported key but not for other diagrams:

---
pie1: ...
---
pie

And from an LSP perspective, it would be a nightmare, and the user wouldn't use it since it would provide auto-complete with combining all possible keys, not just the current diagram.

So I guess this is the way to go:

---
title: awesome title
sankey:
  - width: 300
quadrant:
  - chartWidth: 400
---
sankey

Just in case: if I am not mistaken this --- between config and graph definition is a must.

Yes, it is. I have forgotten it by mistake.

@nirname
Copy link
Contributor

nirname commented Jul 11, 2023

If by some reason we would allow to define multiple diagrams per one image the second option (specifying diagram type) would work too. Do we have an issue for adding this yaml-configs?

@nirname nirname added Type: Enhancement New feature or request and removed Type: Bug / Error Something isn't working or is incorrect labels Jul 11, 2023
@Yokozuna59
Copy link
Member

Do we have an issue for adding this yaml-configs?

Not as far as I'm aware of.

@sidharthv96
Copy link
Member

Would like to hear @aloisklink's take on this.

re: having the diagram key in yaml, it should be fine. Removing the key is gonna make things more complicated.

@Yokozuna59

This comment was marked as resolved.

@nirname nirname changed the title Sankey-beta missing diagram-specific configuration and parsing error Diagram specific configuration with YAML (Sankey-beta missing diagram-specific configuration and parsing error) Jul 12, 2023
@nirname
Copy link
Contributor

nirname commented Jul 12, 2023

I decided to renamed the issue to generalize it and keep the discussion, because it is not about Sankey diagrams at all.

@sidharthv96
Copy link
Member

@Yokozuna59 the triple dash block is the standard of frontmatter. We shouldn't deviate from that.

https://jekyllrb.com/docs/front-matter/

@aloisklink
Copy link
Member

The YAML frontmatter is currently only used for diagram metadata (like title/description). Because it runs as a pre-processing step before any diagram parsing, I think it should only have keys that apply to every diagram, so NO diagram specific code in it (at least not without some large refactoring).

Sticking a MermaidConfig there under a config flag should be an easy thing to implement, though, e.g. something like:

---
title: My example diagram
config: # or should we call this `init` since that's what the current %%{init: syntax uses?
  sankey:
    width: 300
---

Another important thing, I think the YAML front-matter shouldn't throw an error if it sees invalid keys (maybe just a warning). The main reason for this is because if a diagram is written for Mermaid v10.5.0, somebody might try to display this same diagram on a different program that uses an older version of Mermaid, that won't recognize the keys.

@sidharthv96
Copy link
Member

or should we call this init since that's what the current %%{init: syntax uses?

config is more intuitive.

@nirname
Copy link
Contributor

nirname commented Jul 13, 2023

Where to put title attribute for the diagram then? Now we have config.schema.yaml and there is no such attribute as title. That means that only a part under config key is covered by schema. Should not we have another schema on top of that?

@Yokozuna59
Copy link
Member

Where to put title attribute for the diagram then? Now we have config.schema.yaml and there is no such attribute as title. That means that only a part under config key is covered by schema. Should not we have another schema on top of that?

Maybe a types like this should be fine?

type Metadata = {
  title?: string;
  accTitle?: string;
  accDescr?: string;
}
type PossibleDiagramConfig = {
  Record<string, Record<string, number | string | boolean>>;
}
type YAML = MermaidConfig & PossibleDiagramConfig & Metadata;

Although some attributes in MermaidConfig isn't supported in %%{}%%, so not sure if we should support them all in yaml config, e.g., themeCSS. I guess omitting them should resolve this.

@aloisklink
Copy link
Member

I'm thinking we'd have a file like diagram-frontmatter.d.ts (ideally auto-generated from a JSON Schema YAML like diagram-frontmatter.schema.yaml) that looks like:

import type {MermaidConfig} from "./config.types.js";

type DiagramFrontmatter = {
  title?: string;
  accTitle?: string;
  accDescr?: string;
  config?: MermaidConfig;
};

The diagram config is already defined as part of the MermaidConfig (e.g. config: {myDiagram: {...}}.

Defining it as diagram-frontmatter.schema.yaml will give us nice auto-generated docs, and will let us do runtime checking that we have the correct types, and print warnings if types don't match.

@nirname nirname added Status: Pending Is not to be executed as it currently is Area: Development and removed Status: Triage Needs to be verified, categorized, etc labels Jul 15, 2023
@Yokozuna59
Copy link
Member

@Yokozuna59 the triple dash block is the standard of frontmatter. We shouldn't deviate from that.

https://jekyllrb.com/docs/front-matter/

@sidharthv96 While working in the new langium parser, I found out it support multi mode grammar (the ability to apply some rule on specific parts). To use the multi-mode grammar, there should be starting (pushing) and ending (popping) rules to switch between modes.

I could to set the --- part to switch to yaml mode, but I can't use the same rule to switch back mermaid (regular) mode. see langium example.

That's why using ... would be handy here.


Just to make sure, we officially dropping directives, right? So we don't have to create a mode for it.

@sidharthv96
Copy link
Member

sidharthv96 commented Jul 19, 2023

No, we are not dropping directives. We are deprecating them, so no new features will be added to them.
All new configs will go into the YAML frontmatter.

To be clear, we NEVER intentionally break backwards compatibility in syntax. We can make breaking JS-API changes in major releases, but we don't break old syntax (except when using the -beta keyword).


The YAML frontmatter is currently stripped away from the code before it's sent to the parser, so you can ignore it for now. We can add frontmatter support once we get the diagrams running?

nexeck added a commit to nexeck/hugo-theme-relearn that referenced this issue Jul 28, 2023
mermaid is deprecating directives for new diagram types: mermaid-js/mermaid#4630
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Development Status: Pending Is not to be executed as it currently is Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants