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

Align generated MermaidConfig with schema #5515

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

sidharthv96
Copy link
Member

📑 Summary

This matches the generated MermaidConfig type with the schema.

Previously, we were removing required to make all fields optional.

Now, we're using a PartialMermaidConfig type using PartialDeep instead.

📏 Design Decisions

Few alternatives considered.

  1. Use RequiredDeep internally
    Abandoned, as some types in the schema are actually undefined. So using RequiredDeep will cause runtime errors.

  2. Change all fields inside schema to not have undefined.
    Abandoned as it'll be a HUGE change, pulling in default values that are scattered throughout the code. This should be done later, and this PR can be considered as the first step to remove unnecessary default variables inside the code. This would also remove the ability of the authors to actually have undefined if they wanted to in the schema.

  3. Match Generated type with Schema, create fully optional type for external use.

Avoids all the problems above, we just need to coerce some test types internally, which is fine, as the test would fail if there are any issues.

Currently, there is a breaking change, that the type names differ. We can switch the names, so that MermaidConfig will be the exported type which has fully partial props.

Didn't do it now, as that would mean a larger diff, making it harder to review.

📋 Tasks

Make sure you

Copy link

netlify bot commented May 13, 2024

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit ea7d28b
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/664e38e01000e6000859f9e2
😎 Deploy Preview https://deploy-preview-5515--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.

Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 31.50685% with 50 lines in your changes are missing coverage. Please review.

Project coverage is 5.74%. Comparing base (0326d89) to head (159d85e).
Report is 46 commits behind head on develop.

Current head 159d85e differs from pull request most recent head ea7d28b

Please upload reports for the commit ea7d28b to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5515   +/-   ##
=======================================
  Coverage     5.74%   5.74%           
=======================================
  Files          277     277           
  Lines        41967   41955   -12     
  Branches       515     515           
=======================================
  Hits          2409    2409           
+ Misses       39558   39546   -12     
Flag Coverage Δ
unit 5.74% <31.50%> (+<0.01%) ⬆️

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

Files Coverage Δ
packages/mermaid/src/diagram-api/detectType.ts 67.07% <100.00%> (ø)
packages/mermaid/src/utils/subGraphTitleMargins.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagram-api/frontmatter.ts 0.00% <0.00%> (ø)
packages/mermaid/src/preprocess.ts 0.00% <0.00%> (ø)
.../mermaid/scripts/create-types-from-json-schema.mts 0.00% <0.00%> (ø)
packages/mermaid/src/config.ts 51.60% <73.33%> (-0.63%) ⬇️
packages/mermaid/src/diagrams/c4/c4Diagram.ts 0.00% <0.00%> (ø)
...s/mermaid/src/diagrams/flowchart/flowDiagram-v2.ts 0.00% <0.00%> (ø)
...ages/mermaid/src/diagrams/flowchart/flowDiagram.ts 0.00% <0.00%> (ø)
packages/mermaid/src/utils.ts 41.60% <62.50%> (ø)
... and 2 more

@sidharthv96 sidharthv96 marked this pull request as ready for review May 13, 2024 06:48
@sidharthv96 sidharthv96 requested review from aloisklink and knsv and removed request for aloisklink May 15, 2024 15:59
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.

Currently, there is a breaking change, that the type names differ. We can switch the names, so that MermaidConfig will be the exported type which has fully partial props.

👍 Maybe we can call the partial one MermaidConfig, and the one with defaults MermaidConfigWithDefaults or ConfigWithDefaults.


Other than figuring out how to export the partial config type to users, this PR looks okay to me.

@sidharthv96 sidharthv96 marked this pull request as draft May 22, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants