Split docs into separate package

sidharthv96 commented 1 year ago

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 https://github.com/mermaid-js/mermaid/pull/4356#discussion_r1181309626_

Yokozuna59 commented 1 year ago

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

aloisklink commented 1 year ago

How about also splitting the parser into a separate package? And provide AST with it? https://github.com/mermaid-js/mermaid/issues/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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/mermaid-js/mermaid/pull/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 commented 1 year ago

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 commented 1 year ago

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

Yokozuna59 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/mermaid-js/mermaid/blob/10a66030b9979ce127f745c8ddb70922c63a94ca/packages/mermaid/src/Diagram.ts#L59-L65

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 commented 1 year ago

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

https://github.com/mermaid-js/mermaid/blob/10a66030b9979ce127f745c8ddb70922c63a94ca/packages/mermaid/src/Diagram.ts#L59-L65

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 commented 1 year ago

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

aloisklink commented 1 year ago

Replying here instead of in the new issue (https://github.com/mermaid-js/mermaid/issues/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

My 2 cents.

I think that we should remove docs (compiled version) folder from this repository, and move it to a separate one. Keeping it there equals to having dist in the repository along with code. Do not forget to reconfigure github pages to this new repository. Then we also need to remove this warning

Why:

The less branching in instructions the less room for error
Fewer file in repo
'docs' can be ignored in git and becomes stateless, you can easily remove it, alike node_modules
No need to setup different folders for development and production in vitepress, simpler configuration
Easier to search in editor and repository on github, no need to exclude docs folder while searching, no confusion
Bot will not commit to your branch, cleaner history
No need to tell every docs developer to enable workflows on forks to turn this bot on

aloisklink commented 1 year ago

I think that we should remove docs (compiled version) folder from this repository.

I'm :+1: for completely removing it too. Or even better, moving packages/mermaid/src/docs into the docs folder.

I think the main reason why we haven't is that it's because if we have a Mermaid code block like:

flowchart TD
    A[Hello] --> B(World)

GitHub will render the Mermaid diagram, but there's no way to see the Mermaid source-code for a diagram. So my gut feeling is to wait and hope GitHub adds this feature.

By the way, @nirname, do you want to make a new issue for removing the docs/ folder? Even though I personally feel like we might want to hold off on doing it until GitHub changes their Mermaid diagram preview, it's probably worth having a separate issue to track/discuss it, since it's not exactly the same as this issue.

nirname commented 1 year ago

@aloisklink I do not fully understand how is it related to Github mermaid preview, could you elaborate more on that? Does anyone use github for searching for compiled docs? There is https://mermaid.js.org/ for that... I am not asking to move docs to separate repository comletely, only a compiled version of it

aloisklink commented 1 year ago

@nirname, I've replied to your comment in https://github.com/mermaid-js/mermaid/issues/4605 :)

We should probably keep discussion for deleting the docs/ folder in that issue so we don't keep on pinging the other participants on off-topic discussion :)

I've got half-a-mind to hide every comment in this issue for being off-topic, since I think only the OP comment is about moving packages/mermaid/src/docs into something like packages/mermaid. Every other comment is about something else: either about converting the Mermaid parsers to Langium instead if JISON, or removing docs/. :rofl:

mermaid-js / mermaid

Split docs into separate package #4382