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: Support config in frontmatter. #4750

Merged
merged 15 commits into from
Aug 22, 2023
Merged

Conversation

sidharthv96
Copy link
Member

@sidharthv96 sidharthv96 commented Aug 20, 2023

📑 Summary

Allows defining diagram specific config from the frontmatter.

---
config:
  sankey:
    nodeAlignment: justify
---
sankey-beta
%%data

Resolves #4630

📏 Design Decisions

The config will be added using the existing addDirective function, which has the necessary security checks.

📋 Tasks

Make sure you

@sidharthv96
Copy link
Member Author

Related to #4748

@github-actions github-actions bot added the Type: Enhancement New feature or request label Aug 20, 2023
@codecov
Copy link

codecov bot commented Aug 20, 2023

Codecov Report

Merging #4750 (3944e9a) into develop (6e0f411) will increase coverage by 29.27%.
The diff coverage is 52.84%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #4750       +/-   ##
============================================
+ Coverage    45.55%   74.83%   +29.27%     
============================================
  Files           53      146       +93     
  Lines         6621    14561     +7940     
  Branches        32      586      +554     
============================================
+ Hits          3016    10896     +7880     
+ Misses        3604     3549       -55     
- Partials         1      116      +115     
Flag Coverage Δ
e2e 80.88% <85.00%> (?)
unit 45.55% <17.07%> (+<0.01%) ⬆️

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

Files Changed Coverage Δ
packages/mermaid/src/directiveUtils.ts 37.80% <ø> (+26.31%) ⬆️
packages/mermaid/src/mermaidAPI.ts 61.38% <37.50%> (+17.69%) ⬆️
packages/mermaid/src/config.ts 74.49% <40.00%> (+15.76%) ⬆️
packages/mermaid/src/diagram-api/frontmatter.ts 62.26% <53.12%> (+14.76%) ⬆️
packages/mermaid/src/utils.ts 59.53% <55.38%> (+21.40%) ⬆️
packages/mermaid/src/Diagram.ts 66.66% <100.00%> (+23.80%) ⬆️
packages/mermaid/src/assignWithDepth.ts 94.28% <100.00%> (+4.28%) ⬆️
packages/mermaid/src/defaultConfig.ts 47.21% <100.00%> (+2.97%) ⬆️

... and 134 files with indirect coverage changes

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.

Two comments:

  • I don't think we should be changing packages/mermaid/src/config.ts. Can't we just treat config in the YAML frontmatter the same way we treat %%{init: ...} directives?
    That way we can use the already existing addDirective() function, and we don't need to worry about potential security vulnerabilities in this PR. (we may also need to call directiveSanitizer() first).
  • This PR needs documentation on the https://mermaid.js.org/ website (maybe on the https://mermaid.js.org/config/configuration.html page?)

@sidharthv96 sidharthv96 marked this pull request as draft August 20, 2023 16:39
@sidharthv96
Copy link
Member Author

sidharthv96 commented Aug 20, 2023

Can't we just treat config in the YAML frontmatter the same way we treat %%{init: ...} directives?

Yes, that was my first approach. But directives are currently not typed properly. So tried this as a POC.

Docs will be added before merge.

@aloisklink
Copy link
Member

Yes, that was my first approach. But directives are currently not typed properly.

Is there any reason why can't just change the addDirective() function so that it is typed properly? E.g. something like:

diff --git a/packages/mermaid/src/config.ts b/packages/mermaid/src/config.ts
index e0066382..a7e6c49b 100644
--- a/packages/mermaid/src/config.ts
+++ b/packages/mermaid/src/config.ts
@@ -8,10 +8,10 @@ export const defaultConfig: MermaidConfig = Object.freeze(config);
 
 let siteConfig: MermaidConfig = assignWithDepth({}, defaultConfig);
 let configFromInitialize: MermaidConfig;
-let directives: any[] = [];
+let directives: MermaidConfig[] = [];
 let currentConfig: MermaidConfig = assignWithDepth({}, defaultConfig);
 
-export const updateCurrentConfig = (siteCfg: MermaidConfig, _directives: any[]) => {
+export const updateCurrentConfig = (siteCfg: MermaidConfig, _directives: MermaidConfig[]) => {
   // start with config being the siteConfig
   let cfg: MermaidConfig = assignWithDepth({}, siteCfg);
   // let sCfg = assignWithDepth(defaultConfig, siteConfigDelta);
@@ -188,7 +188,7 @@ export const sanitize = (options: any) => {
  *
  * @param directive - The directive to push in
  */
-export const addDirective = (directive: any) => {
+export const addDirective = (directive: MermaidConfig) => {
   if (directive.fontFamily) {
     if (!directive.themeVariables) {
       directive.themeVariables = { fontFamily: directive.fontFamily };

We may have to add add // @ts-expect-error comments, but that's okay, as long as we're improving the existing TypeScript situation (e.g. removing any types).

@sidharthv96
Copy link
Member Author

Is there any reason why can't just change the addDirective() function so that it is typed properly?

No reason at all. I guess my brain was in POC mode and didn't want to touch any existing stuff.

I've cleaned it up now :)

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.

No reason at all. I guess my brain was in POC mode and didn't want to touch any existing stuff.

I've cleaned it up now :)

The implementation looks great to me, all it needs is:

  • E2E tests
    • Can you add a test that has both a config directive in YAML frontmatter, and in a %%{init: } block, just to confirm it works?
  • an example in the demos/ folder (or multiple examples), and
  • some documentation in the https://mermaid.js.org site!

If you want to be really fancy, you can rewrite the your commits to split up the refactor commits (which have no visible changes to users), fix: commits (which fix potential bugs), and the feat: commits (which add new features), e.g. something like refactor: sanitize in addDirective, refactor: improve addDirective types, ... fix: improve YAML frontmatter type handling, feat: support config directives in YAML, but this is a pretty small PR, so it doesn't matter too much 😄

FYI, I think this PR will close issue #4630! If you agree, feel free to stick it in your PR's description.

@sidharthv96 sidharthv96 self-assigned this Aug 21, 2023
@sidharthv96 sidharthv96 marked this pull request as ready for review August 21, 2023 09:34
@knsv knsv added this pull request to the merge queue Aug 22, 2023
@knsv knsv removed this pull request from the merge queue due to a manual request Aug 22, 2023
@knsv knsv added this pull request to the merge queue Aug 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 22, 2023
…rConfig

* origin/develop:
  chore: Remove duplicate CI action
  chore: Add circular dependency check in CI
  refactor: Remove circular dependencies
@sidharthv96 sidharthv96 added this pull request to the merge queue Aug 22, 2023
@sidharthv96
Copy link
Member Author

Merging myself as @knsv has approved and attempted a merge.

Merged via the queue into develop with commit adfed1e Aug 22, 2023
21 checks passed
fuxingloh referenced this pull request in fuxingloh/contented Aug 28, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [mermaid](https://togithub.com/mermaid-js/mermaid) | [`10.3.1` ->
`10.4.0`](https://renovatebot.com/diffs/npm/mermaid/10.3.1/10.4.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/mermaid/10.4.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/mermaid/10.4.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/mermaid/10.3.1/10.4.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/mermaid/10.3.1/10.4.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>mermaid-js/mermaid (mermaid)</summary>

###
[`v10.4.0`](https://togithub.com/mermaid-js/mermaid/releases/tag/v10.4.0)

[Compare
Source](https://togithub.com/mermaid-js/mermaid/compare/v10.3.1...v10.4.0)

#### Features

- feat: Support config in frontmatter. by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[https://github.com/mermaid-js/mermaid/pull/4750](https://togithub.com/mermaid-js/mermaid/pull/4750)
- feat(sankey): Show values by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[https://github.com/mermaid-js/mermaid/pull/4748](https://togithub.com/mermaid-js/mermaid/pull/4748)

#### Docs

- docs: Add development example page. by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[https://github.com/mermaid-js/mermaid/pull/4714](https://togithub.com/mermaid-js/mermaid/pull/4714)
- Documentation for
[#&#8203;2509](https://togithub.com/mermaid-js/mermaid/issues/2509) by
[@&#8203;jason-curtis](https://togithub.com/jason-curtis) in
[https://github.com/mermaid-js/mermaid/pull/4740](https://togithub.com/mermaid-js/mermaid/pull/4740)
- Fixes to Docs sidebar, main page and badges by
[@&#8203;nirname](https://togithub.com/nirname) in
[https://github.com/mermaid-js/mermaid/pull/4742](https://togithub.com/mermaid-js/mermaid/pull/4742)
- Split development documentation into several pages by
[@&#8203;nirname](https://togithub.com/nirname) in
[https://github.com/mermaid-js/mermaid/pull/4744](https://togithub.com/mermaid-js/mermaid/pull/4744)
- Docs: update Latest News section by
[@&#8203;huynhicode](https://togithub.com/huynhicode) in
[https://github.com/mermaid-js/mermaid/pull/4768](https://togithub.com/mermaid-js/mermaid/pull/4768)

#### Chores

- Update all minor dependencies (minor) by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/mermaid-js/mermaid/pull/4732](https://togithub.com/mermaid-js/mermaid/pull/4732)
- Update all patch dependencies (patch) by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/mermaid-js/mermaid/pull/4731](https://togithub.com/mermaid-js/mermaid/pull/4731)
- convert `assignWithDepth` to TS by
[@&#8203;Yokozuna59](https://togithub.com/Yokozuna59) in
[https://github.com/mermaid-js/mermaid/pull/4717](https://togithub.com/mermaid-js/mermaid/pull/4717)
- convert `diagrams/common/svgDrawCommon.js` to ts by
[@&#8203;Yokozuna59](https://togithub.com/Yokozuna59) in
[https://github.com/mermaid-js/mermaid/pull/4724](https://togithub.com/mermaid-js/mermaid/pull/4724)
- ci(release-drafter): add more release notes categories by
[@&#8203;aloisklink](https://togithub.com/aloisklink) in
[https://github.com/mermaid-js/mermaid/pull/4752](https://togithub.com/mermaid-js/mermaid/pull/4752)
- chore(deps): update all patch dependencies (patch) by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/mermaid-js/mermaid/pull/4753](https://togithub.com/mermaid-js/mermaid/pull/4753)
- standardized pie definitions by
[@&#8203;Yokozuna59](https://togithub.com/Yokozuna59) in
[https://github.com/mermaid-js/mermaid/pull/4501](https://togithub.com/mermaid-js/mermaid/pull/4501)
- Remove Circular Dependencies by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[https://github.com/mermaid-js/mermaid/pull/4761](https://togithub.com/mermaid-js/mermaid/pull/4761)
- chore: Enforce type imports by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[https://github.com/mermaid-js/mermaid/pull/4763](https://togithub.com/mermaid-js/mermaid/pull/4763)
- chore: Preview PRs with mermaid-live-editor on Netlify by
[@&#8203;sidharthv96](https://togithub.com/sidharthv96) in
[https://github.com/mermaid-js/mermaid/pull/4769](https://togithub.com/mermaid-js/mermaid/pull/4769)

#### New Contributors

- [@&#8203;jason-curtis](https://togithub.com/jason-curtis) made their
first contribution in
[https://github.com/mermaid-js/mermaid/pull/4740](https://togithub.com/mermaid-js/mermaid/pull/4740)

**Full Changelog**:
mermaid-js/mermaid@v10.3.1...v10.4.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/levaintech/contented).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi41Ni4wIiwidXBkYXRlZEluVmVyIjoiMzYuNTYuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
3 participants