Handle non declared classes #5222

jgreywolf · 2024-01-22T18:13:24Z

📑 Summary

This fix allows classes that have not yet been defined elsewhere to be used in an action such as creating a link, setting style/css class or annotations.

Resolves #2234, #2021

📏 Design Decisions

Used existing solution of calling addClass(className) before accessing getClasses

📋 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

netlify · 2024-01-22T18:13:29Z

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	`5482d0a`
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/65af4d2634664a0008313dac
😎 Deploy Preview	https://deploy-preview-5222--mermaid-js.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

…eeded

sidharthv96

The implementation is good, but I'm not really sure about the feature.
Should we really be adding links, notes, styles etc to classes that have not been created? Would throwing an error make more sense?

If we release it, we can't revert it.

Approve on the code changes, On the fence about the feature.

sidharthv96 · 2024-01-23T05:48:03Z

packages/mermaid/src/diagrams/class/classDb.ts

+export const getClass = function (className: string): ClassNode {
+  return classes[className];
 };


Optional: This would be a big change, but if we use this function everywhere instead of direct dictionary access, we'd only have to add the addClass function here, and also handle any future checks easily.

I'm not sure if this will work, because there are places where we want to create the class, if it does not exist, and places where we don't want to create the class, if it doesn't exist.

Maybe a getOrCreateClass() function might help, though?

aloisklink

~~Agree with @sidharthv96, I'm not sure if we should add this feature.~~

~~But I'm also unsure if throwing an error is okay, since there might be a bunch of diagrams out in the wild that already have this currently ignored syntax.~~

Edit: Actually, if this is syntax that is already allowed (it's just being ignored), I think it's probably okay to add this feature. After all, we can't change this behaviour to instead throw an error if people already have this syntax in existing diagrams. Flowcharts also seem to work similarly, where using style before a node is first created is fine.

Btw, currently, it doesn't work with generic classes:

classDiagram
    style Square fill:#f9f,stroke:#333,stroke-width:4px

    class Square~Shape~{
        int id
        List~int~ position
        setPoints(List~int~ points)
        getPoints() List~int~
    }

renders:

while

classDiagram
    class Square~Shape~{
        int id
        List~int~ position
        setPoints(List~int~ points)
        getPoints() List~int~
    }

    style Square fill:#f9f,stroke:#333,stroke-width:4px

renders

aloisklink · 2024-01-23T15:11:01Z

packages/mermaid/src/diagrams/class/classDb.ts

-    if (classes[id] !== undefined) {
-      classes[id].link = utils.formatUrl(linkStr, config);
+    addClass(className);
+    if (classes[className] !== undefined) {


This if() statement will always be true, since we always call addClass(className) before it.

But it might be required to satisfy typescript.

Hmmmmm, that's a good point. We could do something like:

function addClass(className: string): asserts classes[className] !== undefined {

See https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#assertion-functions

The issue is that calling addClass('Array~string~') creates classes['Array'], not classes['Array~string~'], so we'd probably need a new function like addClassID.

addClass('Array~string~') is SUPPOSED to create classes['Array']. The type is a property on the class object, and not part of the id.

At least, that is the way it is now. PR #5223 will change this so the generic type will be appended to the id. addClass(Array~string~) will create classes['Array-string']

aloisklink · 2024-01-23T15:12:19Z

packages/mermaid/src/diagrams/class/classDb.ts

@@ -458,6 +470,7 @@ export const addClassesToNamespace = function (id: string, classNames: string[])
 };

 export const setCssStyle = function (id: string, styles: string[]) {
+  addClass(id);
  const thisClass = classes[id];
  if (!styles || !thisClass) {


thisClass will never be falsy, since we just created it with addClass.

aloisklink · 2024-01-23T15:12:57Z

packages/mermaid/src/diagrams/class/classDb.ts

@@ -308,6 +320,7 @@ const setClickFunc = function (_domId: string, functionName: string, functionArg
  }

  const id = domId;
+  addClass(id);
  if (classes[id] !== undefined) {


classes[id] !== undefined will never be falsey, since we just called addClass().

aloisklink · 2024-01-23T15:13:42Z

packages/mermaid/src/diagrams/class/classDb.ts

    }
-    if (classes[id] !== undefined) {
-      classes[id].cssClasses.push(className);
+    if (classes[className] !== undefined) {


This if() statement will always be true, since we always call addClass(className) before it.

I removed one of these cases - but forgot to look for any others ;)

aloisklink · 2024-01-23T15:20:12Z

packages/mermaid/src/diagrams/class/classDb.ts

+export const getClass = function (className: string): ClassNode {
+  return classes[className];
 };


I'm not sure if this will work, because there are places where we want to create the class, if it does not exist, and places where we don't want to create the class, if it doesn't exist.

Maybe a getOrCreateClass() function might help, though?

jgreywolf · 2024-01-28T01:03:01Z

I moved this back to draft, because there are changes in PR #5223 I want to use here, so will wait until that one is merged

jgreywolf · 2024-12-16T23:40:26Z

This is no longer relevant

Handle non declared classes

a22e419

jgreywolf requested review from sidharthv96 and aloisklink January 22, 2024 18:13

github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Jan 22, 2024

jgreywolf linked an issue Jan 22, 2024 that may be closed by this pull request

Line order influences syntactic validity #2021

Open

Merge branch 'develop' into bug/2234_class-diagram-call-addClass-if-n…

5482d0a

…eeded

sidharthv96 reviewed Jan 23, 2024

View reviewed changes

aloisklink reviewed Jan 23, 2024

View reviewed changes

jgreywolf marked this pull request as draft January 28, 2024 01:02

jgreywolf closed this Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle non declared classes #5222

Handle non declared classes #5222

jgreywolf commented Jan 22, 2024 •

edited

Loading

netlify bot commented Jan 22, 2024 •

edited

Loading

sidharthv96 left a comment

sidharthv96 Jan 23, 2024

aloisklink Jan 23, 2024

aloisklink left a comment •

edited

Loading

aloisklink Jan 23, 2024

sidharthv96 Jan 23, 2024

aloisklink Jan 24, 2024

jgreywolf Jan 28, 2024

aloisklink Jan 23, 2024

aloisklink Jan 23, 2024

aloisklink Jan 23, 2024

jgreywolf Jan 23, 2024

aloisklink Jan 23, 2024

jgreywolf commented Jan 28, 2024

jgreywolf commented Dec 16, 2024

Handle non declared classes #5222

Handle non declared classes #5222

Conversation

jgreywolf commented Jan 22, 2024 • edited Loading

📑 Summary

📏 Design Decisions

📋 Tasks

netlify bot commented Jan 22, 2024 • edited Loading

✅ Deploy Preview for mermaid-js ready!

sidharthv96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloisklink left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgreywolf commented Jan 28, 2024

jgreywolf commented Dec 16, 2024

jgreywolf commented Jan 22, 2024 •

edited

Loading

netlify bot commented Jan 22, 2024 •

edited

Loading

aloisklink left a comment •

edited

Loading