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

chore: Deprecate mermaidAPI #4821

Merged
merged 17 commits into from
Jun 28, 2024
Merged

chore: Deprecate mermaidAPI #4821

merged 17 commits into from
Jun 28, 2024

Conversation

sidharthv96
Copy link
Member

@sidharthv96 sidharthv96 commented Sep 8, 2023

📑 Summary

Deprecates the use of mermaidAPI.
Update docs to remove references to mermaidAPI.
Add docs for mermaid object.

mermaid.parse and mermaid.render should be used instead of mermaid.mermaidAPI.parse and mermaid.mermaidAPI.render.

📏 Design Decisions

Using MermaidAPI directly could cause issues, and making it internal lets us make changes easily.

📋 Tasks

Make sure you

@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit c1052bd
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/667d5ca54810400008d54789
😎 Deploy Preview https://deploy-preview-4821--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.

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Attention: Patch coverage is 5.55556% with 17 lines in your changes missing coverage. Please review.

Project coverage is 5.74%. Comparing base (ba0d91d) to head (c1052bd).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #4821   +/-   ##
=======================================
  Coverage     5.73%   5.74%           
=======================================
  Files          279     280    +1     
  Lines        42022   41932   -90     
  Branches       517     492   -25     
=======================================
- Hits          2409    2408    -1     
+ Misses       39613   39524   -89     
Flag Coverage Δ
unit 5.74% <5.55%> (+<0.01%) ⬆️

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

Files Coverage Δ
packages/mermaid/src/utils.ts 41.54% <100.00%> (-0.07%) ⬇️
packages/mermaid/src/accessibility.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/mindmap/mindmapDb.ts 0.00% <0.00%> (ø)
...es/mermaid/src/diagrams/mindmap/mindmapRenderer.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/mindmap/svgDraw.ts 0.00% <0.00%> (ø)
packages/mermaid/src/mermaidAPI.ts 0.00% <0.00%> (ø)
packages/mermaid/src/mermaid.ts 0.00% <0.00%> (ø)

... and 1 file 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.

Deprecating the init, mermaidAPI.render, mermaidAPI.initialize, mermaidAPI.parse functions make sense to me, although I'm uncertain if it makes sense to deprecate the entire mermaidAPI object. My comments:

  • Instead of @deprecate-ing the entire mermaidAPI object, would it make more sense to just @deprecate mermaidAPI.render and mermaidAPI.initialize and mermaidAPI.parse? The other functions in mermaidAPI don't seem to have replacements (and if they do have replacements, it's probably easier to describe the replacements in a separate @deprecated Use <...> instead line).

  • We shouldn't mark the mermaidAPI as @internal if it might be being used by third-parties. Using @internal might even hide the @deprecated warning for users.

  • If we're @deprecate-ing functions that may be used by people (and I think it's likely that some people are using these functions), we might want to hold back on deprecating these functions until a v11 release.
    The eslint-plugin-deprecation has 350,000 weekly downloads, so it's quite possible that adding @deprecated will be a breaking change for some users.

    Although, SemVer just states we should make a MINOR release for deprecations, so 🤷, maybe it's overkill to wait until a MAJOR release.

  • We probably should also make a Deprecation label for this PR, and add a Deprecated entry in https://github.com/mermaid-js/mermaid/blob/develop/.github/release-drafter.yml.

@sidharthv96
Copy link
Member Author

Instead of @deprecate-ing the entire mermaidAPI object, would it make more sense to just @deprecate mermaidAPI.render and mermaidAPI.initialize and mermaidAPI.parse? The other functions in mermaidAPI don't seem to have replacements (and if they do have replacements, it's probably easier to describe the replacements in a separate @deprecated Use <...> instead line).

Only getDiagramFromText and globalReset seems like the candidate to expose (via mermaid.ts) in the rest of mermaidAPI.
Everything regarding setting config can be handled by initialize.

The intention is to provide mermaid.ts as the only "official" entrypoint into mermaid, so that we have more flexibility internally to change things, while keeping the public API stable (with low surface area).

@knsv
Copy link
Collaborator

knsv commented Sep 11, 2023

Instead of @deprecate-ing the entire mermaidAPI object, would it make more sense to just @deprecate mermaidAPI.render and mermaidAPI.initialize and mermaidAPI.parse? The other functions in mermaidAPI don't seem to have replacements (and if they do have replacements, it's probably easier to describe the replacements in a separate @deprecated Use <...> instead line).

Only getDiagramFromText and globalReset seems like the candidate to expose (via mermaid.ts) in the rest of mermaidAPI. Everything regarding setting config can be handled by initialize.

The intention is to provide mermaid.ts as the only "official" entrypoint into mermaid, so that we have more flexibility internally to change things, while keeping the public API stable (with low surface area).

We turned it around then. The idea with the mermaidAPI was to have one entry point into Mermaid, which users using their own bootstraps/integrations could use. The mermaid.js was just one reference implementation.

I am little concerned by the potentially braking change though. This might be better in v11

@sidharthv96 sidharthv96 changed the base branch from develop to next September 12, 2023 11:19
@aloisklink
Copy link
Member

Instead of @deprecate-ing the entire mermaidAPI object, would it make more sense to just @deprecate mermaidAPI.render and mermaidAPI.initialize and mermaidAPI.parse? The other functions in mermaidAPI don't seem to have replacements (and if they do have replacements, it's probably easier to describe the replacements in a separate @deprecated Use <...> instead line).

Only getDiagramFromText and globalReset seems like the candidate to expose (via mermaid.ts) in the rest of mermaidAPI. Everything regarding setting config can be handled by initialize.

The intention is to provide mermaid.ts as the only "official" entrypoint into mermaid, so that we have more flexibility internally to change things, while keeping the public API stable (with low surface area).

There doesn't seem to be any replacements for mermaid.mermaidAPI.getConfig, but I don't think we need one. Just in case, we should probably do something like:

/**
 * {@inheritDoc ./config.js:getConfig} // not sure if this syntax will work?
 * @deprecated This function may be removed in the future.
 *             Please comment in <GH discussion thread> if you use this feature.
 */
function getConfig() {
  return getConfig();
}

I could see mermaid.mermaidAPI.defaultConfig being useful? E.g. somebody might do something like the following, in order to add their own custom secure keys to the MermaidConfig:

mermaid.initialize({
  secure: [
    ...myCustomSecureFields,
    ...mermaid.mermaidAPI.defaultConfig.secure,
  ],
});

@sidharthv96 sidharthv96 added this to the v11 milestone Nov 25, 2023
@sidharthv96
Copy link
Member Author

sidharthv96 commented Dec 7, 2023

@aloisklink we can add those if someone requests those, we are only deprecating mermaidAPI, not removing it completely. Otherwise, we're increasing our surface area, which will make future changes trickier.

* next: (387 commits)
  build(deps-dev): bump vite from 4.4.9 to 4.4.12
  Changes to .prettierignore 1. Added 'demos/dev/**' to be ignored by Prettier. 2. Added '!/demos/dev/example.html' so that Prettier ensures no one changes the example.html in a way that doesn't obey the Prettier code formatting rules.
  build: use `tsx` instead of `ts-node-esm`
  chore: Downgrade node to 18.18.2
  fix: #5100 Add viewbox to sankey
  chore(deps): update all minor dependencies
  chore: Rename test
  test: Add unit test for generic classname and namespace
  fix: Check if parentCommit is provided
  Split type from generic class name
  Condition of Parent Id Without Merge Commit Added
  Referenced the PmWiki's Cookbook recipe enabling MermaidJs schematics in wiki pages
  test(e2e): fix pie chart E2E tests for PR #4288
  Add dummy commit to trigger GH checks
  chore: Revert unnecessary export
  refactor: Remove unnecessary calculations
  chore: Fix computeWidth function
  chore: Cleanup setupGraphViewbox
  Update docs
  update mermaidAPI to cleanup the text before passing to getDiagramFromText
  ...
@aloisklink
Copy link
Member

@aloisklink we can add those if someone requests those, we are only deprecating mermaidAPI, not removing it completely. Otherwise, we're increasing our surface area, which will make future changes trickier.

In that case, I think we definitely need to at least improve the deprecation message, e.g. explaining that if parse() and render() doesn't cover somebody's use case, they should comment on a GitHub discussion.

A quick search of MermaidApi.getConfig on GitHub shows 152 matches, so I think there is a good chance people may request it.

My gut feeling is also to add a @deprecated message to each function in mermaidAPI as well, but I don't know enough about TypeScript tooling to know if a @deprecated message just on the mermaidAPI object is sufficient.

And we need to remove the @internal flag, otherwise they might not even see the deprecation message!

@knsv
Copy link
Collaborator

knsv commented Dec 7, 2023

Ha from the beginning the mermaidAPI was intended to be the official API to use mermaid. The mermaid bootstrap was one reference implementation suing the API.

Base automatically changed from next to develop March 6, 2024 09:23
* develop: (588 commits)
  Linting
  chore:  temp fix for eslint OOM
  chore: Update error snapshots
  Fix ESLint
  chore: Prettier
  chore: YOLO `pnpm --recursive update`
  chore: Remove commitlint
  Fix flowchart-elk render test
  chore: Add example page link in index
  chore: Minor fixes
  chore: Build docs
  Use develop as base on develop branch.
  Update renovate json
  update link
  update announcement and blog pages
  Remove `--force` flag
  Tweak editor.bash
  update link
  chore: update browsers list
  Update integrations-community: add Drupal and module.
  ...
* develop: (138 commits)
  chore: Update docs
  chore: Add argos token to cypress config.
  chore: Remove cy.get('svg') calls
  chore: fix Argos parallel
  chore: Support local screenshot testing without applitools or argos
  chore: Skip checking drupal links
  chore: Remove reference screenshot generation from E2E
  chore: Remove cytoscape patch.
  chore(argos): disable matchImageSnapshot
  chore(argos): put parallel mode only when necessary
  chore(argos): setup parallel mode
  chore: setup Argos Visual Testing on E2E
  Fixed linters
  Added more specs for elk detector
  Fixed wrong elk detector, check only beginning of the line for diagram keywords
  Removed unused patch
  Fix cytoscape patch
  chore: Remove cytoscape patch.
  docs: fix node version in CONTRIBUTING.md
  Explain line breaks in `sequenceDiagram.md`
  ...
Copy link

argos-ci bot commented Jun 20, 2024

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ✅ No changes detected - Jun 27, 2024, 12:46 PM

* develop: (21 commits)
  chore: update browsers list
  chore(deps): update all patch dependencies
  docs: Added demo diagram of bidirectional arrows for sequence diagrams
  fix(deps): update all patch dependencies
  chore: Update drupal regex
  chore: Update pnpm
  rebuild
  chore: Use string templates
  chore: Fix docs
  fix tests
  update arrow startx position
  move arrowhead adjustment to buildMessageModel
  fixed incorrect spacing, added e2e-test
  Format
  Replace regex with contain match
  Format
  Fix test
  Add charset=UTF-8
  Encode string to UTF-8 before encoding to Base64
  fix arrow pointer x position
  ...
* develop:
  chore: Stricter TS version
* develop:
  chore Add vitest.workspace.js
* develop:
  chore: Prettier format
@sidharthv96 sidharthv96 merged commit ceb487f into develop Jun 28, 2024
28 checks passed
@sidharthv96 sidharthv96 deleted the sidv/deprecateMermaidAPI branch June 28, 2024 06:12
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.

3 participants