v11.0.0 Breaking changes | WIP

[sidharthv96]

All PRs with breaking changes should be pointed to next branch, not develop.
The v11 milestone can be used to track issues and PRs for v11.

[Yokozuna59]

The main issue with the next branch is that it might be outdated sometimes.

While I was working on #4450, it was pointing out to the next branch, which was fine for most of the time, but some essential PRs were merged, e.g., #4504. So I had to point it out to develop and merge changes to the PR branch, but when I repoint it out to next, it starts showing the commits that will be merged to next are 300+ commits, so I redirect it to develop again.

Maybe we should make the merge queue updates next when merging to develop? But not sure how to resolve conflicts their.

[sidharthv96]

I repoint it out to next, it starts showing the commits that will be merged to next are 300+ commits, so I redirect it to develop again.

In those cases, you can merge develop to next, and it'll be fixed. Let's try to manage with manual syncs for a while before we automate it?

[Yokozuna59]

In those cases, you can merge develop to next, and it'll be fixed.

That should resolve it, but at that time I didn't have access to the repo, so I can't do anything about it :)

Let's try to manage with manual syncs for a while before we automate it?

Okay.

[sidharthv96]

Hehe, that's a problem. We could set something up to keep the branches synced. It can't be 100% automated because there will be conflicts, but let's see how it goes.

[aloisklink]

Ideally we should keep the breaking changes as small as possible, to reduce the chance of merge conflicts.

E.g., if you're doing a breaking change and some refactoring, put the refactoring in separate commits so it can be merged to both develop and then next.

Although PR #4450 is a massive one, so it will likely be a constant struggle to fix merge conflicts on that branch. I've generally found that using git rebase is much easier than doing git merge, but it requires some knowledge of the git CLI.

[aloisklink]

Does it make sense to convert the dist/mermaid.min.js and dist/mermaid.js files to use IIFE instead of UMD?

Benefits:

• We can stop using Vite and switch to use ESBuild instead, which doesn't support UMD. This will fix add @mermaid-js/parser separate package #4450 (comment))

Downsides:

• Using require("mermaid/dist/mermaid.min.js"); will no longer be supported (but does anybody do this?)
  <script src="https://cdn.jsdelivr.net/npm/mermaid/dist/mermaid.js"></script> will continue to work.

[sidharthv96]

Previous issues we had when removing UMD. https://github.com/orgs/mermaid-js/discussions/4148

[sidharthv96]

Released https://www.npmjs.com/package/mermaid/v/11.0.0-alpha.1 with IIFE build.

When using <script src="CDN/mermaid.min.js">, mermaid.<function> will have to be replaced with mermaid.default.<function>.

I have added a console error suggesting the change if that error is detected.

[aloisklink]

In my opinion, changing globalThis.mermaid.<function> to globalThis.mermaid.default.<function> would be too big of a breaking change to make, since it might break people's websites if they're using @latest in their CDN. But I think this should be possible to fix, see #4109 (comment)

[sidharthv96]

Your solution resolved the issue. 🥳

[sidharthv96]

v11.0.0-alpha.2 released with the fix.

[aloisklink]

Edit: @Yokozuna59's proposed solution is a bit more work, but I think it's cleaner, and it won't be a breaking change, so it can wait until a future MINOR/PATCH release. See discussion in https://github.com/orgs/mermaid-js/discussions/4710#discussioncomment-6732808.

I want to change the MermaidConfig TypeScript type to actually have required options.

Currently, in our MermaidConfig JSON Schema and documentation, many options are required. However, because all of these values are set to default values, they're optional when calling mermaidAPI.setConfig().

Instead, I propose that we change mermaidAPI.setConfig() to accept something like DeepPartial<MermaidConfig>. We could also make a new type like type MermaidConfigOptions = DeepPartial<MermaidConfig> to make things easier for users to users to migrate to Mermaid v11, they can just switch their MermaidConfig type to MermaidConfigOptions.

Benefits: When trying code, we no longer have to do things like getConfig().diagram?.xxxx ?? defaultValue because TypeScript will know that some values always exist. Instead, we can just write getConfig().diagram.xxxx.

[Yokozuna59]

RequiredDeep works with pie diagram config, but some other diagrams have undefined options that can actually be undefined, so we can't use Required.

Why do they have undefined values? Isn't the defaultConfig should have all values setted?

[aloisklink]

It's rare, but some values do have undefined in the defaultConfig, for example class.nodeSpacing or class.rankSpacing:

mermaid/packages/mermaid/src/schemas/config.schema.yaml

Lines 1168 to 1177 in 99978da

	nodeSpacing:
	type: integer
	minimum: 0
	# should the default value be 50?
	# see https://github.com/mermaid-js/mermaid/blob/7647ae317a7b2130e32777248d25a9c4d24b8f9f/packages/mermaid/src/diagrams/class/classRenderer-v2.ts#L258
	rankSpacing:
	type: integer
	minimum: 0
	# should the default value be 50?
	# see https://github.com/mermaid-js/mermaid/blob/7647ae317a7b2130e32777248d25a9c4d24b8f9f/packages/mermaid/src/diagrams/class/classRenderer-v2.ts#L259

Many diagrams also have useWidth: undefined, which is very annoying since undefined is a not a valid JSON value, so it can't be specified with JSON Schema. I want to change them all to useWidth: false but we'd first have to make sure that nobody has done config.useWidth ?? val first.

---

By the way @Yokozuna59, I think you're right. Having MermaidConfig as all optional, and then using RequiredDeep<MermaidConfig> is probably cleaner. But we would first need to get rid of all of the undefineds in the defaultConfig, so it's a bit more work.

[Yokozuna59]

It's rare, but some values do have undefined in the defaultConfig, for example class.nodeSpacing or class.rankSpacing

From what I can see, both of them has default values in the renderer but aren't set in the schema.

Many diagrams also have useWidth: undefined, which is very annoying since undefined is a not a valid JSON value, so it can't be specified with JSON Schema. I want to change them all to useWidth: false but we'd first have to make sure that nobody has done config.useWidth ?? val first.

I'm not sure I'm following correctly, the useWidth type is a number, the useMaxWidth type is the boolean and it's default value is true in most diagrams (or all not sure).

As for useWidth, then I agree with you, it value is kinda random in each diagram (and also in pie), and some diagrams have width config which I guess interfere with it. Not sure what is the best approach here, but I'm against of leaving them undefined in defaultConfig, or at least on the diagram specific config.

By the way @Yokozuna59, I think you're right. Having MermaidConfig as all optional, and then using RequiredDeep<MermaidConfig> is probably cleaner. But we would first need to get rid of all of the undefineds in the defaultConfig, so it's a bit more work.

I guess the only big issue is with useWidth since it's kinda vague, some diagrams set that value depending on the element it's in.

[aloisklink]

the useWidth type is a number, the useMaxWidth type is a boolean!

Ah, you're right! Sorry, useMaxWidth is by default true for all diagrams, except gitGraph/sankey (although we should probably make them true in the future too?), and useWidth is occasionally explicitly defined as undefined in the defaultConfig.ts file. I think we can align them both in the future, though.

I'm going to explicitly mark my original comment that I no longer think this needs to be a breaking change, so it can wait for a future 11.x release (or even a 10.x release).

[Yokozuna59]

useMaxWidth is by default true for all diagrams, except gitGraph/sankey (although we should probably make them true in the future too?)

I would recommend doing so.

and useWidth is occasionally explicitly defined as undefined in the defaultConfig.ts file. I think we can align them both in the future, though.

What's the approach we're gonna take here?

I'm going to explicitly mark my original comment that I no longer think this needs to be a breaking change, so it can wait for a future 11.x release (or even a 10.x release).

Sounds great, but what should we do about #4501? The only unresolved issue is default DB.

[sidharthv96]

The fix for #4730 might be a breaking change as we'd have to change how the error is thrown, and cleanup the orphan SVG.