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

standardized pie definitions #4501

Merged

Conversation

Yokozuna59
Copy link
Member

@Yokozuna59 Yokozuna59 commented Jun 16, 2023

📑 Summary

Brief description about the content of your PR.

Partial Resolves #4499

📏 Design Decisions

Discussed inside #4499

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 📓 have added documentation (if appropriate)
  • 🔖 targeted develop branch

Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

In future PRs, please try to do the .js -> .ts conversion (rename) in one commit (with git commit -n to skip pre commit hooks), and the actual code changes in another, which will help git keep track of things in a better way.

@Yokozuna59
Copy link
Member Author

In future PRs, please try to do the .js -> .ts conversion (rename) in one commit (with git commit -n to skip pre commit hooks), and the actual code changes in another, which will help git keep track of things in a better way.

Well do.


Please replay on the discussed stuff in #4499 (comment) if you have time. And I'm planning to update pie docs, should it be in this pr or another one?

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2023

Codecov Report

Merging #4501 (e7ee3eb) into develop (b85c011) will increase coverage by 0.05%.
The diff coverage is 96.87%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4501      +/-   ##
===========================================
+ Coverage    77.04%   77.09%   +0.05%     
===========================================
  Files          144      144              
  Lines        14565    14563       -2     
  Branches       586      590       +4     
===========================================
+ Hits         11221    11227       +6     
+ Misses        3233     3224       -9     
- Partials       111      112       +1     
Flag Coverage Δ
e2e 83.97% <97.64%> (+0.07%) ⬆️
unit 45.36% <85.71%> (+0.01%) ⬆️

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

Files Changed Coverage Δ
packages/mermaid/src/diagrams/pie/pieDiagram.ts 100.00% <ø> (ø)
packages/mermaid/src/utils.ts 61.73% <85.71%> (+0.12%) ⬆️
packages/mermaid/src/diagrams/pie/pieDb.ts 96.15% <96.15%> (ø)
packages/mermaid/src/diagrams/pie/pieRenderer.ts 98.18% <98.18%> (ø)
packages/mermaid/src/defaultConfig.ts 47.21% <100.00%> (+0.39%) ⬆️
...s/mermaid/src/diagram-api/diagram-orchestration.ts 39.53% <100.00%> (ø)
packages/mermaid/src/diagrams/pie/pieDetector.ts 90.90% <100.00%> (-0.76%) ⬇️
packages/mermaid/src/diagrams/pie/pieStyles.ts 100.00% <100.00%> (ø)

@Yokozuna59 Yokozuna59 force-pushed the standardized-pie-definitions branch from cf150ab to 9145a9e Compare June 17, 2023 13:32
@Yokozuna59 Yokozuna59 mentioned this pull request Jun 18, 2023
43 tasks
@Yokozuna59
Copy link
Member Author

@sidharthv96 What happened? Why workflows aren't running?

@sidharthv96
Copy link
Member

I'm not sure, maybe GH issue.

@sidharthv96
Copy link
Member

It's because you have conflicts. Sync with develop and push.
I was on mobile earlier, didn't notice the conflict.

@Yokozuna59 Yokozuna59 force-pushed the standardized-pie-definitions branch from af742f7 to 1d0aa76 Compare June 19, 2023 10:36
@Yokozuna59 Yokozuna59 mentioned this pull request Aug 11, 2023
4 tasks
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.

Everything seems to good to me, except changing pieDb.ts to have it's own config that's separate to the main config object.

My gut feeling is that this should be in a separate PR, since it's a pretty significant change, and I'm not entirely convinced it's a good idea. See my comments below:

packages/mermaid/src/diagrams/pie/pieDb.ts Show resolved Hide resolved
Comment on lines 20 to 24
export const DEFAULT_PIE_CONFIG: Required<PieDiagramConfig> = {
useMaxWidth: true,
useWidth: 984,
textPosition: 0.75,
} as const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we should be loading these values from the JSON Schema, if they exist.

That way, all of these default values with be visible in the docs, like https://mermaid.js.org/config/schema-docs/config.html#definitions-group-piediagramconfig

Suggested change
export const DEFAULT_PIE_CONFIG: Required<PieDiagramConfig> = {
useMaxWidth: true,
useWidth: 984,
textPosition: 0.75,
} as const;
import DEFAULT_CONFIG from '../../defaultConfig.js';
export const DEFAULT_PIE_CONFIG: Required<PieDiagramConfig> = DEFAULT_CONFIG.pie;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you that we need load values from JSON Schema, but it's not fully typed and what to do with values that they're undefined? Using Required here would throw errors.

And we need to change the default config to RequiredDeep<MermaidConfig>.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it's not fully typed and what to do with values that they're undefined? Using Required here would throw errors.

You're right. Ideally we also need to fix the defaulConfig.ts types first, but that'd need to wait until Mermaid v11 (see https://github.com/orgs/mermaid-js/discussions/4710#discussioncomment-6709156).

Is it worth doing something like the following instead? Maybe it's overkill since we'll remove most of it once we get the types working, though 🤷. Up to you.

import DEFAULT_CONFIG from '../../defaultConfig.js';

function isValidDefaultPieConfig(x: PieDiagramConfig): x is  Required<PieDiagramConfig> {
  return x.useMaxWidth && x.useWidth && x.textPosition;
};

export const DEFAULT_PIE_CONFIG: Required<PieDiagramConfig> = (() => {
  const defaultConfig = {useWidth: 984, ...DEFAULT_CONFIG.pie};
  if (!isValidDefaultPieConfig(defaultConfig)) {
    throw new Error("DEFAULT_PIE_CONFIG is missing required entries: ", defaultConfig);
  }
  return defaultConfig;
});

And we need to change the default config to RequiredDeep<MermaidConfig>.

This works for Pie diagrams, but it won't work for all diagram configs, because some values actually can be undefined and are undefined be default, see #4501 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth doing something like the following instead? Maybe it's overkill since we'll remove most of it once we get the types working, though shrug. Up to you.

I think it's kinda overengineering to do so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this, we modify

pie: {
...defaultConfigJson.pie,
useWidth: undefined,
},
to have:

  pie: {
    ...defaultConfigJson.pie,
    useWidth: true, // this line probably can be removed, because `true` is the default value
    useMaxWidth: 984, // should this be 1200 instead?
  },

Then, in this file, we just tell TypeScript this is a Required<PieDiagramConfig> object, e.g. something like:

// @ts-expect-error: This is valid, since the `defaultConfig` defined in packages/mermaid/src/defaultConfig.ts has no `undefined` values
const getConfig = (): Required<PieDiagramConfig> => commonGetConfig().pie;

It's not perfect, but you're right, it's probably over-engineering to do anything more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: you also flipped default values of useWidth and useMaxWidth in example :)


  pie: {
    useWidth: 984, // should this be 1200 instead?
  },

As I said before, useWidth is kinda vogue, the width in pie depends on the parent element, e.g., in cypress/integration/rendering/pie.spec.ts the width is 984:

const style = svg.attr('style');
expect(style).to.match(/^max-width: [\d.]+px;$/);
const maxWidthValue = parseFloat(style.match(/[\d.]+/g).join(''));
expect(maxWidthValue).to.eq(984);

But in demos/pie.html, the parent element width here is 1904.

But the previous (apparently) default value is 1200:

if (width === undefined) {
width = 1200;
}

It was discussed here and here.


Then, in this file, we just tell TypeScript this is a Required<PieDiagramConfig> object, e.g. something like:

// @ts-expect-error: This is valid, since the `defaultConfig` defined in packages/mermaid/src/defaultConfig.ts has no `undefined` values
const getConfig = (): Required<PieDiagramConfig> => commonGetConfig().pie;

It's not perfect, but you're right, it's probably over-engineering to do anything more.

Thanks! I'll do as you suggested.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm not sure why we're returning config from current global config.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the schema import expect errors:

// @ts-expect-error This file is automatically generated via a custom Vite plugin
import defaultConfigJson from './schemas/config.schema.yaml?only-defaults=true';

We can change the defaultConfig type into RequiredDeep without errors (although other values are actually undefined) defe406

Copy link
Member

@aloisklink aloisklink Aug 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm not sure why we're returning config from current global config.

In the packages/mermaid/src/config.ts file, the getSiteConfig() returns:

  • The defaultConfig, merged with
  • the global site config (e.g. set by mermaid.initialize())

In the packages/mermaid/src/config.ts file, the getConfig() returns the diagram specific config, e.g.:

  • The defaultConfig, merged with
  • the global site config (e.g. set by mermaid.initialize())
  • the diagram specific config (e.g. set by directives like %%{init: {"flowchart": {"defaultRenderer": "elk"}} }%%

I know it's a bit confusing, but even though getConfig() in packages/mermaid/src/config.ts is globally available, because it's reset in-between diagram renders, and Mermaid renders each diagram sequentially, it actually contains the diagram-specific config that is set in %%{init: directives (and maybe YAML directives once we get it set up).

It's definitely worth changing in a future PR, though, since I know the behavior is pretty weird, and it causes bugs like #4345


We can change the defaultConfig type into RequiredDeep without errors

It's probably best not to do that, since it's not actually correct, since other variables are undefined.

Maybe something like MermaidConfig & {pie: Required<MermaidConfig["pie"]>} could work, if we know that all values in the pie chart are defined by default?

packages/mermaid/src/diagrams/pie/pieDb.ts Outdated Show resolved Hide resolved
packages/mermaid/src/diagram-api/types.ts Outdated Show resolved Hide resolved
@Yokozuna59
Copy link
Member Author

Yokozuna59 commented Aug 15, 2023

@aloisklink if we need to preform a BREAKING CHANGE on config then we might just point merge distinction to next, not merging this PR would lead to delaying the addition of pie langium parser (#4727 is almost done).

Although pointing it into next would mean more conflict when merging later on.

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.

I'm going to mark this PR as approved, since it's blocking you on #4727, and although I still don't like the about giving pieDb.ts it's own config in this PR, I think that the overall PR is great :)

Before merging, if you have time, I'd still prefer to have the DEFAULT_PIE_CONFIG and getConfig() function in pieDb.ts use the old way of doing things (e.g. putting the default config in packages/mermaid/src/defaultConfig.ts and having the pieDb's getConfig() function calling the getConfig() function in develop/packages/mermaid/src/config.ts), although we would have to add a // @ts-expect-error comment to make TypeScript happy. In a future PR, we could then update pieDb.ts to have it's own config.
See https://github.com/mermaid-js/mermaid/pull/4501/files#r1296277525


@aloisklink if we need to preform a BREAKING CHANGE on config

I think we can get away without doing a BREAKING CHANGE in the config! Your idea on doing RequiredDeep<MermaidConfig> has me convinced, we just need to make sure that our defaultConfig has no undefined values. See discussion in https://github.com/orgs/mermaid-js/discussions/4710#discussioncomment-6732808.

Giving the pie chart it's own config will also not be a breaking change, since it would be internal to Mermaid.

Comment on lines 20 to 24
export const DEFAULT_PIE_CONFIG: Required<PieDiagramConfig> = {
useMaxWidth: true,
useWidth: 984,
textPosition: 0.75,
} as const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this, we modify

pie: {
...defaultConfigJson.pie,
useWidth: undefined,
},
to have:

  pie: {
    ...defaultConfigJson.pie,
    useWidth: true, // this line probably can be removed, because `true` is the default value
    useMaxWidth: 984, // should this be 1200 instead?
  },

Then, in this file, we just tell TypeScript this is a Required<PieDiagramConfig> object, e.g. something like:

// @ts-expect-error: This is valid, since the `defaultConfig` defined in packages/mermaid/src/defaultConfig.ts has no `undefined` values
const getConfig = (): Required<PieDiagramConfig> => commonGetConfig().pie;

It's not perfect, but you're right, it's probably over-engineering to do anything more.

@Yokozuna59
Copy link
Member Author

I'm going to mark this PR as approved, since it's blocking you on #4727

Just to clarify, we don't actually need it in #4727; it will be in a separate PR I'll be working on it after it's merged.

Before merging, if you have time, I'd still prefer to have the DEFAULT_PIE_CONFIG and getConfig() function in pieDb.ts use the old way of doing things (e.g. putting the default config in packages/mermaid/src/defaultConfig.ts and having the pieDb's getConfig() function calling the getConfig() function in develop/packages/mermaid/src/config.ts)

Sure!

cypress/integration/rendering/pie.spec.ts Show resolved Hide resolved
@@ -232,7 +234,7 @@ const config: Partial<MermaidConfig> = {
},
pie: {
...defaultConfigJson.pie,
useWidth: undefined,
useWidth: 984,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we switch to exact width and to this strange value? I needed exact width only once, when I failed to calculate this properly...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not strange, it's the default one expected in cypress.

The width defined in pie is possibly undefined, so we use this value if it doesn't exists.

See #4501 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yokozuna59 please don't self-resolve comments like this one. Resolving this comment is fine, as you did an action that the reviewer was asking, and it was straightforward.

In this case, the comment was a question, which you answered correctly. But you don't know if @nirname has seen the answer, or has any follow-up questions. The only way for @nirname to see your answer would be to click on each resolved comment to see which one it was. As you know, some PRs have many comments, and it would not be practical to go through each. :)

Leaving the comments unresolved makes the life of the reviewer easy. In case comments are left unresolved for a long time, other maintainers will handle it (By resolving it, or merging without resolving).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sidharthv96 Okay.

GitHub sends emails to subscribers when new actions occurs, so they'll be notified by an email.
And GitHub would also mark the PR/issue with blue circle showing that it has new actions.

:)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub sends emails to subscribers when new actions occurs, so they'll be notified by an email.

Yeah, and in active PRs, there are many comments and emails going out. And Gmail sometimes collapses the thread. So I, personally, rarely check email for PR comments.

And GitHub would also mark the PR/issue with blue circle showing that it has new actions.

But we won't know which comments were resolved recently when opening the PR. Everything would just be resolved and collapsed.

@sidharthv96 sidharthv96 merged commit 72fa348 into mermaid-js:develop Aug 20, 2023
13 checks passed
@Yokozuna59 Yokozuna59 deleted the standardized-pie-definitions branch August 20, 2023 15:03
fuxingloh pushed a commit to fuxingloh/contented that referenced this pull request 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://github.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://github.com/mermaid-js/mermaid/releases/tag/v10.4.0)

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

#### Features

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

#### Docs

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

#### Chores

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

#### New Contributors

- [@&#8203;jason-curtis](https://github.com/jason-curtis) made their
first contribution in
[mermaid-js/mermaid#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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants