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

Remove default properties before generating the config types #5126

Merged

Conversation

dsogari
Copy link
Contributor

@dsogari dsogari commented Dec 7, 2023

📑 Summary

Currently, some configuration types listed in packages/mermaid/src/config.type.ts appear duplicated (such as XYChartAxisConfig1 and XYChartAxisConfig2). This is caused by an issue on the json-schema-to-typescript package.

This PR addresses the effect of that third-party issue on the generated types.

Resolves #5125

📏 Design Decisions

I added a small routine that recursively removes a given property from the loaded JSON schema. It gets called before generating the TypeScript types, in order to remove the default property. This does not affect the default values loaded at run-time, since those values are obtained through the .vite/jsonSchemaPlugin.ts script.

📋 Tasks

Make sure you

Copy link

netlify bot commented Dec 7, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit d732a14
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65730da2cf22740008f41201
😎 Deploy Preview https://deploy-preview-5126--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: Other Not an enhancement or a bug label Dec 7, 2023
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Merging #5126 (d732a14) into next (aa4bfa0) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             next    #5126   +/-   ##
=======================================
  Coverage   44.50%   44.50%           
=======================================
  Files          25       25           
  Lines        5206     5206           
  Branches       25       25           
=======================================
  Hits         2317     2317           
  Misses       2888     2888           
  Partials        1        1           
Flag Coverage Δ
unit 44.50% <ø> (ø)

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

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.

Good catch! The types look a lot better now :)

@sidharthv96 sidharthv96 added this to the v11 milestone Dec 8, 2023
@sidharthv96 sidharthv96 changed the base branch from develop to next December 8, 2023 10:36
@sidharthv96 sidharthv96 merged commit dc1b2a6 into mermaid-js:next Dec 10, 2023
15 checks passed
Copy link

mermaid-bot bot commented Dec 10, 2023

@diegosogari, Thank you for the contribution!
You are now eligible for a year of Premium account on MermaidChart.
Sign up with your GitHub account to activate.

@dsogari dsogari deleted the chore/5125-fix-duplicate-config-types branch December 13, 2023 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Other Not an enhancement or a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate configuration types
3 participants