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 packet diagram #4839

Merged
merged 31 commits into from
Jan 23, 2024
Merged

feat: Add packet diagram #4839

merged 31 commits into from
Jan 23, 2024

Conversation

sidharthv96
Copy link
Member

@sidharthv96 sidharthv96 commented Sep 14, 2023

📑 Summary

Adds packet diagram
Resolves #948

TODO:

  • Add title support
  • Add accessibility support
  • Fix theming
  • Add docs
image

📏 Design Decisions

The syntax was adapted from the existing one at http://blockdiag.com/en/nwdiag/packetdiag-examples.html#structure-of-tcp-header

📋 Tasks

Make sure you

@netlify
Copy link

netlify bot commented Sep 14, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 5cc20b5
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65b013a3a144c700088dffd7
😎 Deploy Preview https://deploy-preview-4839--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.

@github-actions github-actions bot added the Type: Enhancement New feature or request label Sep 14, 2023
@sidharthv96 sidharthv96 self-assigned this Sep 14, 2023
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (ce9a9db) 44.50% compared to head (5cc20b5) 44.38%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #4839      +/-   ##
==========================================
- Coverage   44.50%   44.38%   -0.13%     
==========================================
  Files          25       25              
  Lines        5206     5227      +21     
  Branches       25       25              
==========================================
+ Hits         2317     2320       +3     
- Misses       2888     2906      +18     
  Partials        1        1              
Flag Coverage Δ
unit 44.38% <25.00%> (-0.13%) ⬇️

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

Files Coverage Δ
packages/mermaid/src/defaultConfig.ts 45.65% <100.00%> (+0.59%) ⬆️
packages/mermaid/src/themes/theme-dark.js 2.74% <10.00%> (-0.07%) ⬇️
packages/mermaid/src/themes/theme-forest.js 3.81% <18.18%> (-0.10%) ⬇️

Copy link
Member

@Yokozuna59 Yokozuna59 left a comment

Choose a reason for hiding this comment

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

Sorry for making all comments in this review request changes. Some of them are just comments, but the review got so big that I couldn't separate them.

There's two files I haven't fully review db.ts and renderer.ts. I'll review them when I've time.

And please mock the packet renderer in __mocks__ and here packages/mermaid/src/mermaidAPI.spec.ts.

demos/packet.html Outdated Show resolved Hide resolved
packages/parser/src/index.ts Show resolved Hide resolved
packages/parser/src/language/packet/packet.langium Outdated Show resolved Hide resolved
packages/parser/src/language/packet/packet.langium Outdated Show resolved Hide resolved
packages/parser/src/language/packet/packet.langium Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/packet/types.ts Outdated Show resolved Hide resolved
packages/mermaid/src/schemas/config.schema.yaml Outdated Show resolved Hide resolved
packages/mermaid/src/schemas/config.schema.yaml Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/packet/db.ts Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/packet/styles.ts Outdated Show resolved Hide resolved
@sidharthv96
Copy link
Member Author

Thanks for the detailed review @Yokozuna59 !
This was like a first-draft with the new system, and actually, my first new diagram in mermaid :)
You've made some excellent points, will fix these :)

packages/mermaid/src/schemas/config.schema.yaml Outdated Show resolved Hide resolved
packages/mermaid/src/defaultConfig.ts Outdated Show resolved Hide resolved
packages/mermaid/src/defaultConfig.ts Outdated Show resolved Hide resolved
@Yokozuna59 Yokozuna59 self-requested a review September 24, 2023 11:53
sidharthv96 and others added 4 commits November 14, 2023 11:21
* next: (193 commits)
  Update all patch dependencies
  Fix docs
  Update packages/mermaid/src/docs/community/questions-and-suggestions.md
  Update packages/mermaid/src/diagrams/class/classRenderer-v2.ts
  update edge ids
  draw top actors with lines  first followed by messages
  Bump GitHub workflow actions to latest versions
  Update docs
  Documentation: clarify sentence
  Fix lint
  Fix typo
  fix typo
  Add new Atlassian integrations
  chore(deps): update all patch dependencies
  Update demos/sequence.html
  chore: release v10.6.1
  fix
  fix
  fix: render the participants in same order as they are created
  fix(flow): fix invalid ellipseText regex
  ...
Moved some types around
Removed unnecessary params

Co-authored-by: Reda Al Sulais <[email protected]>
Co-authored-by: Alois Klink <[email protected]>
Move populate into parser

Co-authored-by: Reda Al Sulais <[email protected]>
@sidharthv96 sidharthv96 marked this pull request as ready for review November 14, 2023 19:35
* next:
  fix text-decoration for abstract attibutes
  ci(pr-labeler): add required `template` option
  ci(pr-labeler): replace TimonVS/pr-labeler-action
  style(pr-labeler): format .github/pr-labeler.yml
  docs(ci/pr-labeler): warn about security issues
  ci(release-draft): handle new release-drafter name
  ci(release-drafter): remove unused `branch` config
  ci(pr-labeler): limit GITHUB_TOKEN permissions
  ci(release-draft): limit GITHUB_TOKEN permissions
@github-actions github-actions bot added the Type: Enhancement New feature or request label Nov 15, 2023
Copy link
Member

@Yokozuna59 Yokozuna59 left a comment

Choose a reason for hiding this comment

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

It would be better if add another image for the diagram with title in PR description.

I'm not sure about this, but shouldn't the title be at the top of the diagram, or it expected to be at the bottom?

And two more things:

And please mock the packet renderer in __mocks__ and here packages/mermaid/src/mermaidAPI.spec.ts.

#4839 (review)

And maybe add some test cases in packages/parser/tests for packet diagram.

packages/parser/src/index.ts Show resolved Hide resolved
packages/mermaid/src/diagrams/packet/renderer.ts Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/packet/renderer.ts Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/packet/packet.spec.ts Outdated Show resolved Hide resolved
@sidharthv96
Copy link
Member Author

And please mock the packet renderer in mocks and here packages/mermaid/src/mermaidAPI.spec.ts.

@Yokozuna59 @aloisklink @nirname Are the files except d3.ts in __mocks__ actually used?
I removed all of them in 96ae4a5, and nothing broke locally, or in CI.
Maybe a vitest update made them redundant?

image

@aloisklink
Copy link
Member

Are the files except d3.ts in __mocks__ actually used?

Looking at #3808, I believe all of these diagram mocks are only being used for the following test:

describe('render', () => {
// These are more like integration tests right now because nothing is mocked.
// But it is faster that a cypress test and there's no real reason to actually evaluate an image pixel by pixel.
// render(id, text, cb?, svgContainingElement?)
// Test all diagram types. Note that old flowchart 'graph' type will invoke the flowRenderer-v2. (See the flowchart v2 detector.)
// We have to have both the specific textDiagramType and the expected type name because the expected type may be slightly different than was is put in the diagram text (ex: in -v2 diagrams)
const diagramTypesAndExpectations = [
{ textDiagramType: 'C4Context', expectedType: 'c4' },
{ textDiagramType: 'classDiagram', expectedType: 'classDiagram' },
{ textDiagramType: 'classDiagram-v2', expectedType: 'classDiagram' },
{ textDiagramType: 'erDiagram', expectedType: 'er' },
{ textDiagramType: 'graph', expectedType: 'flowchart-v2' },
{ textDiagramType: 'flowchart', expectedType: 'flowchart-v2' },
{ textDiagramType: 'gitGraph', expectedType: 'gitGraph' },
{ textDiagramType: 'gantt', expectedType: 'gantt' },
{ textDiagramType: 'journey', expectedType: 'journey' },
{ textDiagramType: 'pie', expectedType: 'pie' },
{ textDiagramType: 'requirementDiagram', expectedType: 'requirement' },
{ textDiagramType: 'sequenceDiagram', expectedType: 'sequence' },
{ textDiagramType: 'stateDiagram-v2', expectedType: 'stateDiagram' },
];
describe('accessibility', () => {
const id = 'mermaid-fauxId';
const a11yTitle = 'a11y title';
const a11yDescr = 'a11y description';
diagramTypesAndExpectations.forEach((testedDiagram) => {
describe(`${testedDiagram.textDiagramType}`, () => {
const diagramType = testedDiagram.textDiagramType;
const diagramText = `${diagramType}\n accTitle: ${a11yTitle}\n accDescr: ${a11yDescr}\n`;
const expectedDiagramType = testedDiagram.expectedType;
it('should set aria-roledscription to the diagram type AND should call addSVGa11yTitleDescription', async () => {
const a11yDiagramInfo_spy = vi.spyOn(accessibility, 'setA11yDiagramInfo');
const a11yTitleDesc_spy = vi.spyOn(accessibility, 'addSVGa11yTitleDescription');
await mermaidAPI.render(id, diagramText);
expect(a11yDiagramInfo_spy).toHaveBeenCalledWith(
expect.anything(),
expectedDiagramType
);
expect(a11yTitleDesc_spy).toHaveBeenCalled();
});
});
});
});
});

But it sounds like the mocks are mainly there to force diagram.draw() to return an empty string instead of undefined, but the diagram.draw() function is only supposed to return undefined anyway, so it does look like this mock files aren't needed anymore:

/**
* Type for function draws diagram in the tag with id: id based on the graph definition in text.
*
* @param text - The text of the diagram.
* @param id - The id of the diagram which will be used as a DOM element id.
* @param version - MermaidJS version from package.json.
* @param diagramObject - A standard diagram containing the DB and the text and type etc of the diagram.
*/
export type DrawDefinition = (
text: string,
id: string,
version: string,
diagramObject: Diagram
) => void | Promise<void>;

It seems safe to me, but if you think it's high risk, you could make the change in a separate PR so we could more easily revert it?

Copy link
Member

@Yokozuna59 Yokozuna59 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Just a minor question: when will we drop v10? This is the first diagram; we only have a language parser for it, so users can't use it (unless they use the preview link).

@sidharthv96
Copy link
Member Author

@Yokozuna59 we're planning to release v11 soon.

The plan is to merge your Pie PR first, update this one as necessary, then start the release process (testing, docs, etc).

* next: (316 commits)
  Lint
  Remove echo
  RefTest
  Echo event
  Update cypress
  Fix applitools
  Fix applitools
  docs: fix lint
  docs: move community to Discord
  Swap condition blocks to avoid using negation
  Reposition const declaration to ideal place
  Change repetitive values into consts
  docs: fix swimm link
  Fix Update Browserslist
  Use pnpm/action-setup@v2
  Fix lint
  Cleanup e2e.yml
  Ignore push events on merge queue
  Remove ::
  Remove ::
  ...
@sidharthv96 sidharthv96 merged commit b7c72cb into next Jan 23, 2024
18 of 19 checks passed
@sidharthv96 sidharthv96 deleted the feat/948_packetDiagram branch January 23, 2024 19:35
@sidharthv96 sidharthv96 mentioned this pull request Jul 26, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants