making consitent config imports from diagramAPI #4889

dreathed · 2023-10-01T14:29:00Z

📑 Summary

A very important issue, since the local dev / test environment does not necessarily reflect production env when config is involved!

This PR changes all imports of config.js in the diagrams and the dagre-wrapper folder to importing the same functions (getConfig etc.) from diagramAPI. DiagramAPI is set to import the getConfig, setConfig etc. functions from config and exports them again.

Honestly I am not sure why it solves the issue but it does. Probably it has something to do with module resolution...
I could not verify @aloisklink 's reasons for this bug given here.

As far as I can see this only effects the local test build and local e2e tests. I assume the bundling for production is done a bit differently?
This PR helps to sync production - as seen in mermaidLive - and local dev testing.
In production (as in mermaidLive) this is not an issue (see below), I assume the bundling is done differently than for local testing.

Resolves #4345

📏 Design Decisions

Importing from a kind of proxi file (diagramAPI) instead of directy from config.js in directories dagre-wrapper and diagrams.

📋 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

Additional Information

I ran into this issue while working on something else, trying to inject a configuration object into the createText function. I was super confused, because somehow I could not get it to work until I found out that getConfig returns sometimes the global config and sometimes a newly initialized defaultConfig. This depends on where getConfig is called from.

I also mentioned that there are failing local e2e tests on the unchanged develop branch. This PR solves this too.

To see that there is some reinitialization going on, just put a console.log in the global scope of config.ts. Run dev server and see the console of your test page: Your message gets logged twice! Once from mermaid and once from config (see the files in the browser of this issues description)

The problem can also be recreated by changing the flowchart.htmlLabels config in your local copy of dev/example.html:

see the car icon in the bottom right

<html>
  <head>
    <title>Mermaid development page</title>
    <link
      rel="stylesheet"
      href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.4.2/css/all.min.css"
    />
  </head>
  <body>
    <pre id="diagram" class="mermaid">
      ---
      title: myTitle
      config:
        flowchart:
          htmlLabels: false
      ---
      flowchart TD
        A["`Christmas a very long long line with no line breaks this is almost impossible how long this line is`"] -->|Get money| B(Go shopping)
        B --> C{Let me think}
        C -->|One| D[Laptop]
        C -->|Two| E[iPhone]
        C -->|Three| F[fa:fa-car Car]
    </pre>

    <div id="dynamicDiagram"></div>
    <script type="module">
      import mermaid from '/mermaid.esm.mjs';
      mermaid.parseError = function (err, hash) {
        console.error('Mermaid error: ', err);
      };
      mermaid.initialize({
        startOnLoad: true,
        logLevel: 0,
        flowchart: {htmlLabels: false}
      });
      const value = `graph TD\nHello --> World`;
      const el = document.getElementById('dynamicDiagram');
      const { svg } = await mermaid.render('dd', value);
      console.log(svg);
      el.innerHTML = svg;
    </script>
  </body>
</html>

leads to the local output:

While in the liveEditor the config:

{
  "theme": "dark",
  "flowchart": {
    "htmlLabels": false
  }
}

leads to this correct output:

After the changes of the PR the local test file looks like it should:

After the PR we get a funny diagram-API file from the transpiler:

The imports of getConfig are still sometimes from diagramAPI and somtimes from mermaid, but since the config is not reset it works.

This solution does not seem very elegant, but it solves the issue...

netlify · 2023-10-01T14:29:04Z

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	`497ffde`
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/6522d862e5ae500008061824
😎 Deploy Preview	https://deploy-preview-4889--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.

codecov · 2023-10-01T14:33:09Z

Codecov Report

Merging #4889 (497ffde) into develop (12a4707) will decrease coverage by 0.57%.
The diff coverage is 81.25%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4889      +/-   ##
===========================================
- Coverage    79.97%   79.40%   -0.57%     
===========================================
  Files          164      164              
  Lines        13623    13815     +192     
  Branches       693      693              
===========================================
+ Hits         10895    10970      +75     
- Misses        2579     2696     +117     
  Partials       149      149

Flag	Coverage Δ
e2e	`85.27% <77.19%> (+<0.01%)`	⬆️
unit	`42.94% <84.61%> (-0.36%)`	⬇️

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

Files	Coverage Δ
packages/mermaid/src/dagre-wrapper/clusters.js	`88.23% <ø> (ø)`
packages/mermaid/src/dagre-wrapper/createLabel.js	`92.85% <ø> (ø)`
packages/mermaid/src/dagre-wrapper/edges.js	`83.97% <ø> (ø)`
packages/mermaid/src/dagre-wrapper/nodes.js	`89.62% <ø> (ø)`
packages/mermaid/src/dagre-wrapper/shapes/note.js	`100.00% <ø> (ø)`
packages/mermaid/src/dagre-wrapper/shapes/util.js	`96.22% <ø> (ø)`
packages/mermaid/src/diagram-api/diagramAPI.ts	`70.58% <100.00%> (-19.89%)`	⬇️
packages/mermaid/src/diagrams/c4/c4Db.js	`65.27% <100.00%> (ø)`
packages/mermaid/src/diagrams/c4/c4Renderer.js	`88.08% <100.00%> (ø)`
...ges/mermaid/src/diagrams/class/classRenderer-v2.ts	`79.85% <ø> (ø)`
... and 28 more

... and 2 files with indirect coverage changes

aloisklink

Looks good to me!

Weird how changing the import fixes it. I wonder if updating Vite would help, since we're currently on version 4.3.9, which is a couple of months old:

mermaid/pnpm-lock.yaml

Lines 178 to 180 in 1d9ce74

    
           vite: 
        
             specifier: ^4.3.9 
        
             version: 4.3.9(@types/node@18.16.0)

By the way, once this gets merged (we can do this in another PR), do you think it makes sense to add an ESLint no-restricted-imports rule like the following so that this doesn't happen again:

"no-restricted-imports": ["error", {
    "patterns": [{
      "group": ["*/config.js"],
      "message": "Please import config functions from diagram-api/diagramAPI.js instead, see https://github.com/mermaid-js/mermaid/pull/4889."
    }]
}]

dreathed · 2023-10-08T18:14:23Z

I had some problems resolving merge requests, but now everything should be fine.

@aloisklink I tried the latest vite version, but this did not work.
To make a new linter rule is great! But when I tried that specific rule it catched all imports. If there is a way to only target specific files to apply that rule to, that would be a very good solution!

aloisklink · 2023-10-15T22:53:25Z

@aloisklink I tried the latest vite version, but this did not work.

Drat, oh well, your PR also seems to work too.

Normally I'd leave this PR open for another review, but since this PR has been open for a while, and you've changed a lot of files (which might cause merge conflicts if we leave it open), I'll merge this now! Thanks for the fix.

To make a new linter rule is great! But when I tried that specific rule it catched all imports. If there is a way to only target specific files to apply that rule to, that would be a very good solution!

Hmmmm, it's probably easier to have it run on all files.

Then for we could add a comment like // eslint-disable-line no-restricted-imports on

mermaid/packages/mermaid/src/diagram-api/diagramAPI.ts

Line 8 in 497ffde

} from '../config.js';

But that's something we can do in another PR!

aloisklink approved these changes Oct 4, 2023

View reviewed changes

aloisklink added the Type: Bug / Error Something isn't working or is incorrect label Oct 4, 2023

making consitent config imports from diagramAPI

846fb3f

dreathed marked this pull request as draft October 8, 2023 13:18

dreathed force-pushed the bug/4345_bundle_current_config_consitently_in_one_file branch from 42cea82 to 846fb3f Compare October 8, 2023 13:28

dreathed added 2 commits October 8, 2023 15:32

fix classDB

01203d8

undo changes for commonDb to avoid circular imports

497ffde

dreathed marked this pull request as ready for review October 8, 2023 17:48

aloisklink added this pull request to the merge queue Oct 15, 2023

Merged via the queue into mermaid-js:develop with commit d04f4c2 Oct 15, 2023
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

making consitent config imports from diagramAPI #4889

making consitent config imports from diagramAPI #4889

dreathed commented Oct 1, 2023

netlify bot commented Oct 1, 2023 •

edited

Loading

codecov bot commented Oct 1, 2023 •

edited

Loading

aloisklink left a comment

dreathed commented Oct 8, 2023

aloisklink commented Oct 15, 2023 •

edited

Loading

making consitent config imports from diagramAPI #4889

making consitent config imports from diagramAPI #4889

Conversation

dreathed commented Oct 1, 2023

📑 Summary

📏 Design Decisions

📋 Tasks

Additional Information

see the car icon in the bottom right

netlify bot commented Oct 1, 2023 • edited Loading

✅ Deploy Preview for mermaid-js ready!

codecov bot commented Oct 1, 2023 • edited Loading

Codecov Report

aloisklink left a comment

Choose a reason for hiding this comment

dreathed commented Oct 8, 2023

aloisklink commented Oct 15, 2023 • edited Loading

netlify bot commented Oct 1, 2023 •

edited

Loading

codecov bot commented Oct 1, 2023 •

edited

Loading

aloisklink commented Oct 15, 2023 •

edited

Loading