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

Add a config validator and organise global configs #3397

Open
Tracked by #3340 ...
sidharthv96 opened this issue Sep 2, 2022 · 10 comments
Open
Tracked by #3340 ...

Add a config validator and organise global configs #3397

sidharthv96 opened this issue Sep 2, 2022 · 10 comments
Assignees

Comments

@sidharthv96
Copy link
Member

sidharthv96 commented Sep 2, 2022

Add a config validator to make sure config is passed in right place & format. (It should also handle transforming legacy config). This would make the internal code a lot cleaner.

All global configs should be grouped together as mentioned in #3337
Related #1172

@github-actions github-actions bot added the Status: Triage Needs to be verified, categorized, etc label Sep 2, 2022
@sidharthv96 sidharthv96 changed the title Add a config validator to make sure config is passed in right place & format. (It should also handle transforming legacy config). This would make the internal code a lot cleaner. Add a config validator and organise global configs Sep 2, 2022
@sidharthv96
Copy link
Member Author

image

Should standardize config naming convention.

@sidharthv96 sidharthv96 self-assigned this Sep 13, 2022
@sidharthv96 sidharthv96 added Area: Development and removed Status: Triage Needs to be verified, categorized, etc labels Sep 13, 2022
@sidharthv96
Copy link
Member Author

sidharthv96 commented Sep 13, 2022

Key points

  • Should not break existing configs
  • Any new config should only be added in the new format
  • Ability to deprecate configs with warning
  • Should convert config to a standard format for all internal use. This should remove edge case checking in many places.
  • Directives should be handled
flowchart
Old["{Old Config+New Config}"] -->Validate 
Validate --> Transform 
Transform --> InternalConfig
Loading

@knsv
Copy link
Collaborator

knsv commented Sep 19, 2022

Let us not forget about the decoupled diagrams. With a decoupled diagram I mean a diagram built outside of Mermaid (core) that is registering itself as a diagram that Mermaid can render. These diagrams will likly have additional configuration parameters etc.

@weedySeaDragon
Copy link
Contributor

@knsv and @sidharthv96 I've posted my thoughts on Themes and configuration in a discussion.
#3495

I think that figuring out how Themes will work & be implemented will answer many of the questions for how Configurations should work & be implemented.

This was referenced Feb 19, 2023
@jgreywolf
Copy link
Contributor

@sidharthv96 Is this issue considered resolved with the merge of the two 10.0 PR listed above?

@sidharthv96
Copy link
Member Author

I don't think so.
We didn't do any major changes to configs in v10.

@sidharthv96 sidharthv96 mentioned this issue Apr 21, 2023
43 tasks
@aloisklink
Copy link
Member

I have a pending PR that will change the MermaidConfig to be defined in a single-file1 using JSONSchema: #4112 (it's got a bunch of merge conflicts that I don't want to fix until Mermaid v10.2.0 gets merged to main, then the main branch gets merged back to develop2)

We would then have a single file where:

  • The default values are located1
  • The types of the values are located (and whether they're optional/required)
    • That PR makes the TypeScript value of every value optional, to match our existing types and to avoid causing any breaking changes.
  • The documentation for values is located

Once that gets merged, we can make the following breaking changes:

  • Use a json-schema-validator like ajv to automatically warn/error about invalid config entries (this is what eslint/webpack uses for validating their configs)
  • Make every config option that is required required in TypeScript, so that we can remove edge-checking, since TypeScript will know these fields will always exist.

Footnotes

  1. Except for non-JSON default values, i.e. functions like FontCalculator. 2

  2. We might want to reconsider our practice of make a release branch off develop, merging it to main, then merging main back to develop. It's a bit unusual, and it seems to cause a bunch of Git conflicts.

@sidharthv96
Copy link
Member Author

@aloisklink, now that #4112 is merged, should we create separate issues for the 2 breaking changes mentioned?

@aloisklink
Copy link
Member

@aloisklink, now that #4112 is merged, should we create separate issues for the 2 breaking changes mentioned?

I think, after talking to @Yokozuna59, we can do be both of these things, without having any breaking changes.

  • Make every config option that is required required in TypeScript, so that we can remove edge-checking, since TypeScript will know these fields will always exist.

I think, after talking with @Yokozuna59 (see https://github.com/orgs/mermaid-js/discussions/4710#discussioncomment-6709156), it probably makes more sense to make everything in the MermaidConfig JSON Schema and TypeScript types optional. If we then make every default value in the MermaidConfig not undefined (but maybe null), then we can make functions like getConfig() return RequiredDeep<MermaidConfig> and we will know that none of the values are undefined.

After we make that change, adding ajv to print warnings will be even easier!

@Yokozuna59
Copy link
Member

Yokozuna59 commented Sep 11, 2023

image

Should standardize config naming convention.

And rename colour to color (since most of them are already color).


And I think we should remove functions from the defaultConfig, they're more like helper functions to be used in renderer rather than separate attribute. We could create those functions in DB instead.

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

No branches or pull requests

6 participants