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: Add diagramType to RenderResult and ParseResult #5118

Merged
merged 10 commits into from
Dec 10, 2023

Conversation

sidharthv96
Copy link
Member

@sidharthv96 sidharthv96 commented Dec 6, 2023

📑 Summary

This allows users to get the type of the diagram without calling an extra function.

Resolves #5117

📏 Design Decisions

The return type of parse changes, so it is a breaking change. We decided that keeping the return type non-homogeneous is the best option in terms of forward and backwards compatibility.

render only has an additional parameter to the return type, so it is unlikely to cause issues.

📋 Tasks

Make sure you

Copy link

netlify bot commented Dec 6, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit b9bc518
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65729d8475311300098976c2
😎 Deploy Preview https://deploy-preview-5118--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.

@sidharthv96 sidharthv96 linked an issue Dec 6, 2023 that may be closed by this pull request
@sidharthv96 sidharthv96 added this to the v11 milestone Dec 6, 2023
@sidharthv96 sidharthv96 added Type: Enhancement New feature or request Breaking Change Breaking change labels Dec 7, 2023
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 things:

  • I'd like the | false to be moved out of the ParseResult type and instead into the ReturnType of parse(), since that will make it easier to work with.
  • Please add a ! into the PR title, e.g. feat!: my description, to further emphasise that there is a breaking change. I'd even recommend changing the name of this PR to something like: feat!: change `parse()` to return object on valid diagrams to make it more obvious that there is a breaking change, and what it is.

Other than that, this PR looks good to me!

packages/mermaid/src/docs/config/usage.md Outdated Show resolved Hide resolved
packages/mermaid/src/docs/config/usage.md Outdated Show resolved Hide resolved
packages/mermaid/src/mermaidAPI.ts Outdated Show resolved Hide resolved
Co-authored-by: Alois Klink <alois@aloisklink.com>
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #5118 (b9bc518) 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    #5118   +/-   ##
=======================================
  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.

sidharthv96 and others added 6 commits December 8, 2023 09:39
* next:
  chore: Fix import.meta.vitest warnings
  Update docs
  Update NiceGuy.io links in integrations-community.md
Co-authored-by: Alois Klink <alois@mermaidchart.com>
 into sidv/diagramType

* 'sidv/diagramType' of https://github.com/mermaid-js/mermaid:
  Update docs
  Update packages/mermaid/src/docs/config/usage.md
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.

Thanks for making those changes, I'll leave it open for a few days before merging, in case anybody else wants to review, but it looks good to me now!

@sidharthv96 sidharthv96 merged commit ff6188b into next Dec 10, 2023
23 checks passed
@sidharthv96 sidharthv96 deleted the sidv/diagramType branch December 10, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Breaking change Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RenderResult should have diagram type
2 participants