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

feat: add @mermaid-js/parser package and info langium parser #4727

Merged
merged 28 commits into from
Aug 28, 2023

Conversation

Yokozuna59
Copy link
Member

@Yokozuna59 Yokozuna59 commented Aug 12, 2023

📑 Summary

  • Create the parser package inside packages/parser.
  • Create info langium parser and integrate it in packages/mermaid.
  • add title, accTitle, and accDesrc to info grammar.
  • ignore yaml, comments, and directives in info grammar

Benchmark (Vitest)

JISON commit: 99978da
Langium commit: 6fa8294 (langium has more possible hidden rules and attributes)

name hz min max mean p75 p99 p995 p999 rme samples
JISON 476,910.58 0.0012 5.7337 0.0021 0.0028 0.0044 0.0058 0.0158 ±0.64% 1000000
Langium 307,517.52 0.0025 0.7103 0.0033 0.0030 0.0069 0.0092 0.0194 ±0.29% 1000000

BREAKING CHANGES

  • Remove showInfo from infoDb.

Issues

📏 Design Decisions

How langium parser works?

sequenceDiagram
actor Package
participant Module
participant TokenBuilder
participant Lexer
participant Parser
participant ValueConverter

Package --> Module: Create services
Note left of TokenBuilder: When to override? For keywords<br>and rules needs custom function
Note left of TokenBuilder: When to reorder? When langium makes<br>rules at the begging because of performance
TokenBuilder --> Module: Override or/and reorder rules (if needed)
TokenBuilder --> Lexer: Read string and transform it to token stream
Lexer --> Parser: Parse token stream into AST
Note left of ValueConverter: For example: remove double<br>whitespace and double quotes
Parser --> ValueConverter: Clean/modify tokenized<br>rules returned value (if needed)
ValueConverter --> Package: Return AST
Loading

From #4727 (comment)

📋 Tasks

Make sure you

@Yokozuna59 Yokozuna59 requested a review from sidharthv96 August 12, 2023 19:29
@Yokozuna59
Copy link
Member Author

The version isn't fully shown:

image

@Yokozuna59 Yokozuna59 force-pushed the add-info-langium-parser branch from a18c0a7 to bf6d957 Compare August 12, 2023 19:37
@Yokozuna59
Copy link
Member Author

How to insert errors into error diagram?

image

@Yokozuna59 Yokozuna59 force-pushed the add-info-langium-parser branch 3 times, most recently from e8620f6 to d4945b4 Compare August 12, 2023 21:15
@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

Merging #4727 (cd19829) into next (9cb62f4) will increase coverage by 1.22%.
Report is 121 commits behind head on next.
The diff coverage is 27.74%.

❗ Current head cd19829 differs from pull request most recent head 4d53136. Consider uploading reports for the commit 4d53136 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #4727      +/-   ##
==========================================
+ Coverage   45.36%   46.58%   +1.22%     
==========================================
  Files          52       55       +3     
  Lines        6649     6818     +169     
  Branches       32       36       +4     
==========================================
+ Hits         3016     3176     +160     
- Misses       3632     3641       +9     
  Partials        1        1              
Flag Coverage Δ
unit 46.58% <27.74%> (+1.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
packages/mermaid/src/Diagram.ts 42.05% <0.00%> (-0.81%) ⬇️
...s/mermaid/src/diagram-api/diagram-orchestration.ts 30.58% <ø> (+0.35%) ⬆️
...ackages/mermaid/src/diagrams/error/errorDiagram.ts 85.71% <ø> (-0.96%) ⬇️
packages/mermaid/src/directiveUtils.ts 9.75% <ø> (-1.74%) ⬇️
packages/mermaid/src/utils.ts 38.22% <13.88%> (+0.15%) ⬆️
packages/mermaid/src/diagram-api/frontmatter.ts 41.50% <24.24%> (-12.84%) ⬇️
packages/mermaid/src/config.ts 60.32% <26.66%> (+1.59%) ⬆️
packages/mermaid/src/mermaidAPI.ts 43.51% <33.33%> (-0.18%) ⬇️
packages/mermaid/src/diagram-api/detectType.ts 66.26% <42.85%> (+14.12%) ⬆️
packages/mermaid/src/assignWithDepth.ts 90.00% <100.00%> (ø)
... and 4 more

... and 2 files with indirect coverage changes

@Yokozuna59 Yokozuna59 force-pushed the add-info-langium-parser branch from e567deb to b77a4fe Compare August 12, 2023 21:56
@Yokozuna59
Copy link
Member Author

Yokozuna59 commented Aug 12, 2023

  • make dev script also use langium-cli to compile grammar and AST when there's a change on parser related files, e.g., .langium files and langium-config.json.

  • set mode to production in langium-cli to minimize generated files.

@Yokozuna59
Copy link
Member Author

I didn't use infoDb because it doesn't match with parser AST, and I think we need to preform some BREAKING CHANGES here.

@sidharthv96 sidharthv96 changed the base branch from next to sidv/esbuildV10 August 13, 2023 09:55
@sidharthv96
Copy link
Member

Changing the base branch so only relevant changes are visible.

@sidharthv96
Copy link
Member

@Yokozuna59 can you provide a highlevel diagram/explanation of how everything works?

@Yokozuna59
Copy link
Member Author

@sidharthv96 It would take time, but sure. But I don't info or even pie diagrams have much weird grammars or DBs that need more explanation or uses all services.

packages/mermaid/package.json Outdated Show resolved Hide resolved
packages/parser/package.json Show resolved Hide resolved
packages/parser/package.json Outdated Show resolved Hide resolved
packages/parser/src/language/info/infoTokenBuilder.ts Outdated Show resolved Hide resolved
packages/parser/src/language/index.ts Outdated Show resolved Hide resolved
packages/parser/src/language/info/infoModule.ts Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/info/infoParser.ts Outdated Show resolved Hide resolved
@sidharthv96 sidharthv96 changed the base branch from sidv/esbuildV10 to sidv/esbuildV11 August 13, 2023 13:24
@sidharthv96 sidharthv96 changed the base branch from sidv/esbuildV11 to sidv/esbuildV10 August 13, 2023 13:24
@sidharthv96
Copy link
Member

sidharthv96 commented Aug 13, 2023

parse.patch
@Yokozuna59 this is what I meant.

// packages/mermaid/src/diagrams/info/infoParser.ts

import { parse, Info } from 'mermaid-parser';
import { log } from '../../logger.js';
import { ParserDefinition } from '../../diagram-api/types.js';

export const parser: ParserDefinition = {
  parse: (input: string): void => {
    const result = parse<Info>('info', input);
    log.debug({ result });
  },
};

parse.patch

image

I feel like we could even remove this parser file entirely, as it's generic. It could be replaced with a processor, that traverses the AST and populates the DB.

Does langium have support for something like https://chevrotain.io/docs/tutorial/step3a_adding_actions_visitor.html#visitor-methods ?

Maybe the returned object itself is processed this, I'm not sure.

@Yokozuna59 Yokozuna59 requested a review from sidharthv96 August 14, 2023 15:08
@Yokozuna59
Copy link
Member Author

@sidharthv96 I think I've resolved somehow, I took some inspiration from code. eb463f7

Note: I didn't modify Diagram yet, let's review current code then modify it.

I feel like we could even remove this parser file entirely, as it's generic. It could be replaced with a processor, that traverses the AST and populates the DB.

The current parse function have every possible AST without default value, we could set the default to be common, then it would be fine if the diagram exists

interface Common {
accDescr?: string;
accTitle?: string;
title?: string;
}

Does langium have support for something like https://chevrotain.io/docs/tutorial/step3a_adding_actions_visitor.html#visitor-methods?

Maybe the returned object itself is processed this, I'm not sure.

I don't know tbh.

packages/parser/src/detector.ts Outdated Show resolved Hide resolved
packages/parser/src/index.ts Outdated Show resolved Hide resolved
packages/parser/src/language/info/index.ts Outdated Show resolved Hide resolved
packages/parser/src/language/mermaid/mermaidModule.ts Outdated Show resolved Hide resolved
@sidharthv96
Copy link
Member

Does langium have support for something like https://chevrotain.io/docs/tutorial/step3a_adding_actions_visitor.html#visitor-methods?

Maybe the returned object itself is processed this, I'm not sure.

I don't know tbh.

image

The playground has the feature.

@Yokozuna59 Yokozuna59 requested a review from sidharthv96 August 14, 2023 16:16
@Yokozuna59
Copy link
Member Author

  • When we're gonna remove unnecessary parameters in draw function?
  • Should we add the parser version to rendered svg since they don't have the same version?

@Yokozuna59
Copy link
Member Author

@Yokozuna59 can you address the open comments?

#4727 (comment) and #4727 (comment)

@Yokozuna59
Copy link
Member Author

@Yokozuna59 Looking pretty good from a Langium/LSP perspective. For the editor, you'll need to ensure in the long-term that all languages/services are available from a single SharedServices instance (via the ServiceRegistry). Currently, each language has their own shared services instance. Aside from that, it looks pretty good 👍

@msujew What about syntax highlighting and railroad? Do we have to generate and export them in packages/parser package? Or we could just export .langium files to let other packages generate those files?

@msujew
Copy link

msujew commented Aug 24, 2023

@Yokozuna59 I'm not sure I would export anything of that at all. Ideally, you would write your own syntactical highlighting file for vscode, since the generated one is very simplistic and not designed to be used in production.

Why would you want to export the railroad diagram? It's for development/documentation purposes and doesn't serve any runtime value.

@Yokozuna59
Copy link
Member Author

Yokozuna59 commented Aug 24, 2023

@Yokozuna59 I'm not sure I would export anything of that at all. Ideally, you would write your own syntactical highlighting file for vscode, since the generated one is very simplistic and not designed to be used in production.

@msujew My main idea is to use it as a base, then modify according to each use case. I don't think we should figure out how to support each change in grammar manually, we could use the ones generated by langium-cli them modify then if needed.
We currently have one for mermaid.live here, but it kind of outdated right now.

@netlify
Copy link

netlify bot commented Aug 26, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 4d53136
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/64ec565eb3584c0008495eda
😎 Deploy Preview https://deploy-preview-4727--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Yokozuna59 Yokozuna59 requested a review from sidharthv96 August 26, 2023 11:02
@Yokozuna59
Copy link
Member Author

Should we add test cases for title and accessibilities in info? It would increase coverage:

image

image

@sidharthv96
Copy link
Member

@Yokozuna59 can you sync with next?
If there aren't any critical issues to fix in this PR, I'd love to merge it, as it has been open for a while.

Requesting final reviews from @knsv @aloisklink @nirname @mermaid-js/atlantians

@Yokozuna59
Copy link
Member Author

@Yokozuna59 can you sync with next?

It's synced with next, the netlilfy workflows are failing because of eslint out of memory, could be related to #4363.

If there aren't any critical issues to fix in this PR

There aren't any issues as far as I know, but some attributes in the package aren't exported that are essential for the language server.

@jgreywolf
Copy link
Contributor

Did I see a comment that the netlify stuff is broken, and that is "OK" for now?

@Yokozuna59
Copy link
Member Author

@jgreywolf I don't think it would bother us right now.

@Yokozuna59 Yokozuna59 linked an issue Aug 27, 2023 that may be closed by this pull request
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.

Everything looks great to me, with one exception!

the netlilfy workflows are failing because of eslint out of memory

If netlilfy is failing, that's pretty major! (sorry, @Yokozuna59, I know this isn't your fault!)

Can we somehow let netlilfy use more memory? gatsbyjs/gatsby#15190 (comment) seems to suggest we can just add the following to our netlify.toml file:

[build.environment]
# prevent out-of-memory error when building
NODE_OPTIONS = "--max_old_space_size=4096"

Comment on lines +2 to +3
"name": "mermaid-parser",
"version": "0.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

I'll publish it to the org before merging. Then you can deprecate the old one.

Sounds good! Remember to also update the mermaid-parser links in the README.md!

packages/mermaid/src/Diagram.ts Outdated Show resolved Hide resolved
* next:
  chore: Increase heap size when building
  fix(er): bug if relationship is declared first
  test(er): add cypress test on entity name alias
  feat(er): use square brackets to add aliases
  docs(er): add release version for entity name aliases
  feat(er): add entity name alias
@sidharthv96
Copy link
Member

Netlify should be fixed now.

@sidharthv96 sidharthv96 merged commit 44b93c0 into mermaid-js:next Aug 28, 2023
@sidharthv96
Copy link
Member

Congratulations @Yokozuna59 ! You really pushed this through, we've been considering replacing JISON for a looong time, without actually doing it.

Massive thanks to @msujew and langium team for the support!

@Yokozuna59 Yokozuna59 deleted the add-info-langium-parser branch August 28, 2023 09:01
@Yokozuna59
Copy link
Member Author

Yokozuna59 commented Aug 28, 2023

@sidharthv96 Thanks! Many thanks to @msujew and the Langium team for the massive support and fast response; I wouldn't have made it without them :)
And of course I wouldn't forget the support from @sidharthv96 ;)


I've some questions:

  • When should we export more types/functions from packages/parser to be able to use it for the LS?
  • When would the @mermaid-js/parser package be released? So I could deprecate the current and refer to the new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide parser as separate module
6 participants