-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
base: develop
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
* develop: chore: remove unrefSubSchemas
There was a problem hiding this 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.
📑 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.
Use RequiredDeep internally
Abandoned, as some types in the schema are actually undefined. So using RequiredDeep will cause runtime errors.
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.
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
MERMAID_RELEASE_VERSION
is used for all new features.develop
branch