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

[v11] Limit MermaidConfig enum TypesScript types to certain values #4803

Conversation

aloisklink
Copy link
Member

📑 Summary

When implementing #4112, we skipped adding proper TypeScript types for MermaidConfig values that were enums, as in they could only be certain specific values. This is because we didn't want to cause a breaking change for any users of our Typescript types.

In the next MAJOR release of Mermaid, we should remove these types.

For example, const config: MermaidConfig = {flowchart: {defaultRenderer: "this-renderer-does-not-exist"}}; will now throw a TypeScript error, when currently it's okay.

THIS PR IS A BREAKING CHANGE to TypeScript types.

📏 Design Decisions

I had to slightly modify some test files to make TypeScript happy, e.g. by adding as const.

Technically, users can just // @ts-ignore any TypeScript errors, or they might just not use TypeScript, so we can't fully trust that every value is valid, though. This will need to wait until we implement #3397, which will actually validate the config at run-time in JavaScript, instead of at build-time with TypeScript.

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines

  • 💻 have added necessary unit/e2e tests.

  • 📓 have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features.

  • 🔖 targeted develop branch

    • BREAKING CHANGE, so I've targeted the next branch instead.

We're planning on limiting some of MermaidConfig's types to specific
values (e.g. `0 | 1` instead of `number`).
Currently (in Mermaid v10), pretty much all enum types in the
MermaidConfig have generic `string` or `number` fallbacks,
for backwards compatibility.

This commit drops this. The MermaidConfig TypeScript types now expects
a limited amount of values.

BREAKING-CHANGE: Remove `MermaidConfig` generic type fallbacks for
                 enum values.
@aloisklink aloisklink added the Breaking Change Breaking change label Sep 2, 2023
@aloisklink aloisklink added this to the v11 milestone Sep 2, 2023
@netlify
Copy link

netlify bot commented Sep 2, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit b48136d
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/64f38abf39fa5f0009c70802
😎 Deploy Preview https://deploy-preview-4803--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.

@aloisklink aloisklink added the Type: Other Not an enhancement or a bug label Sep 2, 2023
@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #4803 (b48136d) into next (6e51f8f) will decrease coverage by 30.51%.
Report is 64 commits behind head on next.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             next    #4803       +/-   ##
===========================================
- Coverage   77.08%   46.57%   -30.51%     
===========================================
  Files         146       55       -91     
  Lines       14573     6819     -7754     
  Branches      592       36      -556     
===========================================
- Hits        11233     3176     -8057     
- Misses       3221     3642      +421     
+ Partials      119        1      -118     
Flag Coverage Δ
e2e ?
unit 46.57% <ø> (+1.01%) ⬆️

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

see 144 files with indirect coverage changes

@sidharthv96 sidharthv96 merged commit fc229cf into mermaid-js:next Sep 3, 2023
@aloisklink
Copy link
Member Author

Hmmmm, weird, I keep getting the following error in the build-docs.yml GitHub Actions CI.

<--- Last few GCs --->

[2409:0x68ff430]    79546 ms: Mark-sweep 1996.0 (2089.1) -> 1993.4 (2089.1) MB, 2047.2 / 0.0 ms  (average mu = 0.391, current mu = 0.014) allocation failure; scavenge might not succeed
[2409:0x68ff430]    82285 ms: Mark-sweep 2009.2 (2089.1) -> 1996.0 (2105.3) MB, 2681.7 / 0.0 ms  (average mu = 0.217, current mu = 0.021) allocation failure; scavenge might not succeed


<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
 1: 0xb83f50 node::Abort() [node]
 2: 0xa94834  [node]
 3: 0xd647c0 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [node]
 4: 0xd64b67 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [node]
 5: 0xf42265  [node]
 6: 0xf5474d v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 7: 0xf2ee4e v8::internal::HeapAllocator::AllocateRawWithLightRetrySlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node]
 8: 0xf30217 v8::internal::HeapAllocator::AllocateRawWithRetryOrFailSlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node]
 9: 0xf113ea v8::internal::Factory::NewFillerObject(int, v8::internal::AllocationAlignment, v8::internal::AllocationType, v8::internal::AllocationOrigin) [node]
10: 0x12d674f v8::internal::Runtime_AllocateInYoungGeneration(int, unsigned long*, v8::internal::Isolate*) [node]
11: 0x17035b9  [node]
Aborted (core dumped)
 ELIFECYCLE  Command failed with exit code 134.
undefined
/home/runner/work/mermaid/mermaid/packages/mermaid:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  [email protected] docs:build:vitepress: `pnpm docs:pre:vitepress && (cd src/vitepress && pnpm run build) && cpy --flat src/docs/landing/ ./src/vitepress/.vitepress/dist/landing`
Exit status 1

Taken from https://github.com/mermaid-js/mermaid/actions/runs/6060110031/attempts/2

It's in the - building client + server bundles... part of docs:build:vitepress script, so it's unrelated to #4363.

@Yokozuna59
Copy link
Member

@aloisklink It may have happened after updating the next branch in this commit: 84f3baf.
In #4799, when I merged next into the PR branch, the build-docs workflow started failing with each push.

@Yokozuna59 Yokozuna59 mentioned this pull request Sep 6, 2023
2 tasks
@aloisklink aloisklink deleted the refactor/mermaid-config-limit-enum-types branch September 7, 2023 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Breaking change Type: Other Not an enhancement or a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants