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

Split docs into separate package #4382

Open
sidharthv96 opened this issue May 8, 2023 · 24 comments
Open

Split docs into separate package #4382

sidharthv96 opened this issue May 8, 2023 · 24 comments
Labels
Area: DevOps Status: Triage Needs to be verified, categorized, etc

Comments

@sidharthv96
Copy link
Member

sidharthv96 commented May 8, 2023

This isn't something for you to do in this PR, but what do you think about the packages/mermaid/src/docs folder into something like packages/docs? It might make it easier to split out what devDependencies we have for mermaid, and what devDependencies we only need for the docs.

Originally posted by @aloisklink in #4356 (comment)

@github-actions github-actions bot added the Status: Triage Needs to be verified, categorized, etc label May 8, 2023
@Yokozuna59
Copy link
Member

How about also splitting the parser into a separate package? And provide AST with it? #2523

@aloisklink
Copy link
Member

How about also splitting the parser into a separate package? And provide AST with it? #2523

That would be quite a lot of work to do, since each diagram has it's own parser.

We'd first have to split each diagram type in it's own package, then split each of those diagrams into their own subpackages (i.e. renderer, parser). But I do like the idea of having each diagram in it's own package too :)

Although, I'd love to have ASTs for each diagram! Currently, a lot of the diagram code is pretty poorly documented, so having well defined ASTs for each parser would make the code a lot easier to work with.

Maybe having each diagram optionally expose a getAST() function? It would still be quite a lot of work to document every diagram's AST though.

@sidharthv96
Copy link
Member Author

@aloisklink I have this plan in the back of my mind to somehow standardize the diagrams internal implementations. Main thing to solve is the issue when running async. If we could create separate diagram objects, then they can run in "parallel", but it could get complicated when DOM access is involved.
This could also help with sharing features between diagrams.

@aloisklink
Copy link
Member

aloisklink commented May 9, 2023

If we could create separate diagram objects, then they can run in "parallel", but it could get complicated when DOM access is involved.

Something like the following should be okay, as long as each .render() is going into a separate DOM element:

const flowchart = new Flowchart(siteConfig);
// contains metadata (e.g. custom MermaidConfig or title/description) and AST
const parseResult = await flowchart.parse(mmd);
const renderedDiagram = await flowchart.render(parseResult, domContainer);

We'd have to make sure each call to render() never modifies the original siteConfig and works on their own config object though.

@Yokozuna59
Copy link
Member

That would be quite a lot of work to do, since each diagram has it's own parser.

I'm aware of that :). But by doing so, it would be much clearer to developers and would help me create the formatter/linter I want. 4362#discussion-5148111

We'd first have to split each diagram type in it's own package, then split each of those diagrams into their own subpackages (i.e. renderer, parser).

By package, do you mean that each has its own package.json? If so, does that mean a different package on npmjs.com? Because I think it might be better if there was one package for parsing (with all its diagrams) with one import rather than separate parser packages for each. If there is a different usage for each maybe exporting them would be fine.

Maybe having each diagram optionally expose a getAST() function?

I think the .parse() method should return an AST, but it must be under a major release since there won't be backward compatibility anymore. So anything would be fine.

... It would still be quite a lot of work to document every diagram's AST though.

Sure thing.

@sidharthv96
Copy link
Member Author

It would be wonderful to have the grammars written using https://langium.org/ (or something similar), which gives us an LSP out of the box.

@Yokozuna59
Copy link
Member

It would be wonderful to have the grammars written using https://langium.org/ (or something similar), which gives us an LSP out of the box.

Oh, so are you planning to migrate from jison? What would you recommend other than langium (never heard of it, tbh)? So that we can list more options and choose the best option for the current situation.

When I was going to create the linter, there weren't any ASTs available for the diagrams. So I thought of modifying the current parsers by adding more data, e.g., location, and so on. Hence, it would be a waste of time to modify the parsers if you were planning to migrate away from them.

@sidharthv96
Copy link
Member Author

sidharthv96 commented May 10, 2023

Jison has been a source of many problems for us. Especially since the current package is unmainatined.
Chevrotain is a good alternative, with a sample PR at #3432
Langium is built on top of Chevrotain, with more features built in. And it felt like writing langium grammar would be more closer to jison than cevrotain code.

@Yokozuna59 the jison migration won't be happening soon. No need to delay other features based on this. Even if we migrate, all current features of diagrams will be supported, and it'll be a gradual change.

@Yokozuna59
Copy link
Member

I'll try implementing the class diagram parsing using it since it's the most familiar diagram I'm familiar with. I don't currently have the time since I've got finals, but I'll try after that in a separate repo.

I might ask for help if there is anything not clear or a better solution or design for it.

@sidharthv96
Copy link
Member Author

We have to consider the impact on size of mermaid too when choosing a suitable parser.

@Yokozuna59
Copy link
Member

We have to consider the impact on size of mermaid too when choosing a suitable parser.

It would be hard to estimate the size without trying to implement it. And by providing AST, it will increase the size of just reimplementing it in another parsing tool.

@Yokozuna59
Copy link
Member

I've created a mini parser using langium (playground).

Just asking to make sure that I'm on the right track.

NOTE: not even have way finished.

@sidharthv96, @aloisklink, Do you have any feedbacks? And please provide with some weird looking valid classDiagram code to consider.

@aloisklink
Copy link
Member

aloisklink commented May 13, 2023

I've created a mini parser using langium (playground).

Just asking to make sure that I'm on the right track. Note: not even have way finished.

@aloisklink, Do you have any feedbacks? And please provide with some weird looking valid classDiagram code to consider.

I'm not really an expert in class diagrams, or jison, so I can't really help you there, but it might be easier to first port over pie diagrams, since that's one of the simplest diagrams, (the current .jison file is only 106 lines: https://github.com/mermaid-js/mermaid/blob/develop/packages/mermaid/src/diagrams/pie/parser/pie.jison).

If you do want to stick with class diagrams, as a good starting point, you can try looking at all the class diagrams in the E2E tests in https://github.com/mermaid-js/mermaid/blob/develop/cypress/integration/rendering/classDiagram-v2.spec.js, and in the class diagram docs.

I'd imagine that the first time we port over a diagram, the actual langium grammar won't matter too much, what's more important is the JavaScript code that handles calling langium and interpreting the results!

@Yokozuna59
Copy link
Member

I'm not really an expert in class diagrams, or jison, so I can't really help you there, but it might be easier to first port over pie diagrams, since that's one of the simplest diagrams, (the current .jison file is only 106 lines: https://github.com/mermaid-js/mermaid/blob/develop/packages/mermaid/src/diagrams/pie/parser/pie.jison).

I guess I will start working on the pie diagram as you suggested. Although I've made more progress on the class diagram than last time, I'm still not done.

you can try looking at all the class diagrams in the E2E tests in https://github.com/mermaid-js/mermaid/blob/develop/cypress/integration/rendering/classDiagram-v2.spec.js, and in the class diagram docs.

Thanks! I'll consider those test cases for this and other diagrams.

I'd imagine that the first time we port over a diagram, the actual langium grammar won't matter too much,

It would be great if we had a starter code for Langium that contained all the common stuff in all diagrams, for example, comments, directives, excluding whitespaces, etc. And since Langium supports imports, there would be less duplicate code than the previous one https://mermaid.js.org/community/newDiagram.html#directives.

what's more important is the JavaScript code that handles calling langium and interpreting the results!

Sure, it's important! To be honest, I don't know how to use the mermaid API yet. Even if I have a well-structured AST, I don't know if there is a method that can take parsed output and create a diagram out of it other than mermaid.parser.parse, which takes code. Can you point how to?


Should I create a separate issue? I think it's not right to continue discussing it here.

@aloisklink
Copy link
Member

Sure, it's important! To be honest, I don't know how to use the mermaid API yet. Even if I have a well-structured AST, I don't know if there is a method that can take parsed output and create a diagram out of it other than mermaid.parser.parse, which takes code. Can you point how to?

I believe the way most of the current Mermaid diagrams work, is that each diagram has a .parse() function:

parse() {
if (this.detectError) {
throw this.detectError;
}
this.db.clear?.();
this.parser.parse(this.text);
}

When that is called, it fills up it's this.db object, which is then used when you call the .render() function.

The pieDb used for pie diagrams is: https://github.com/mermaid-js/mermaid/blob/b7d31adda44d92dde06efeebb5218933c44b7e62/packages/mermaid/src/diagrams/pie/pieDb.js

Since there are a bunch of tests for parsers that check if a given mmd code creates certain db values, it might be easier to easier to write a Langium AST to Mermaid PieDb converter first, instead of modifying a bunch of rendering code and tests.

Should I create a separate issue? I think it's not right to continue discussing it here.

That's a good idea! A GitHub issue has better visibility, but a GitHub Discussion is easier to comment on! Feel free to pick whichever you prefer, but stick a comment here saying something like "I've made a new issue/discussion for using Langium instead of jison for parsing, see <link>"

@Yokozuna59
Copy link
Member

I believe the way most of the current Mermaid diagrams work, is that each diagram has a .parse() function:

parse() {
if (this.detectError) {
throw this.detectError;
}
this.db.clear?.();
this.parser.parse(this.text);
}

When that is called, it fills up it's this.db object, which is then used when you call the .render() function.

The pieDb used for pie diagrams is: https://github.com/mermaid-js/mermaid/blob/b7d31adda44d92dde06efeebb5218933c44b7e62/packages/mermaid/src/diagrams/pie/pieDb.js

Cool, thanks! It must be parsed and rendered in a browser, right? Can I just use it locally?

Since there are a bunch of tests for parsers that check if a given mmd code creates certain db values, it might be easier to easier to write a Langium AST to Mermaid PieDb converter first, instead of modifying a bunch of rendering code and tests.

I'll look into the details later on. I guess I'd create the grammar and parser first, then find a way to integrate it with the Mermaid API. But thanks!

That's a good idea! A GitHub issue has better visibility, but a GitHub Discussion is easier to comment on! Feel free to pick whichever you prefer, but stick a comment here saying something like "I've made a new issue/discussion for using Langium instead of jison for parsing, see "

I guess I'd go with an issue because it might bring more contributions that might help me.

@Yokozuna59
Copy link
Member

I've made a new issue for reimplementing parser in Langium instead of Jison, see #4401.

@aloisklink
Copy link
Member

Replying here instead of in the new issue (#4401), since the question is here

It must be parsed and rendered in a browser, right?

Parsing can be done anywhere (which is why there are unit tests for them in Node.JS).

Rendering usually only works in a browser, since tools like JSDom don't support doing Layout in Node.JS. This is the reason why the E2E/rendering tests are in the cypress/ folder, and are only run with pnpm run e2e (:sob: and they're super slow).

@jgreywolf
Copy link
Contributor

It would be great if we had a starter code for Langium that contained all the common stuff in all diagrams, for example, comments, directives, excluding whitespaces, etc. And since Langium supports imports, there would be less duplicate code than the previous one https://mermaid.js.org/community/newDiagram.html#directives.

This is the most awesome part from my perspective. I started working with jison parser to see about reusing common stuff - and the whole process just seemed clunky

@Yokozuna59
Copy link
Member

This is the most awesome part from my perspective. I started working with jison parser to see about reusing common stuff - and the whole process just seemed clunky

You could help if you wanted. I've got an almost fully functional pie chart parser.

@nirname

This comment was marked as off-topic.

@aloisklink

This comment was marked as off-topic.

@nirname

This comment was marked as off-topic.

@aloisklink

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: DevOps Status: Triage Needs to be verified, categorized, etc
Projects
None yet
Development

No branches or pull requests

5 participants