style: forbid using console in mermaid src code

[aloisklink]

📑 Summary

Adds an eslint rule forbidding using console in the mermaid source code. Instead, the src/logger should be used instead, so that websites can disable logging.

I've also removed the two instances on console.log/console.error that currently exist in the mermaid source code.

Edit: It looks like eslint doesn't scan the .jison files. There were a bunch of active console.logs in c4Diagram.jison, so I've deleted those too. I could also comment them out too if that's preferred. The regex console.log\([^\)]*(JSON.stringify\([^\)]*\))?[^\)]*\); seems to detect them all.

Edit2: I've added a jest-fail-on-console to fail jest tests that use console.log, since ESLint doesn't scan .jison files

Resolves #3339

📏 Design Decisions

I've kept this as allowed in the cypress/ and demos/ folder, because a bunch of .html files still use console.log.

Potential issues:

• console.log can be useful for debugging. yarn test will now throw an error and prevent tests if use a console.log. Instead, you'd have to use yarn jest src/ which is a bit more verbose.
• .jison files aren't scanned by ESLint, so we still need to manually check those files. Edit2: As long as these files are run in Jest, the jest-fail-on-console check should scan for them.
• Instead of deleting all the console.log in the c4Diagram.jison file, should I comment them out instead?

📋 Tasks

Make sure you

• 📖 have read the contribution guidelines
• 💻 have added unit/e2e tests (if appropriate)
• 🔖 targeted develop branch

[sidharthv96]

console.log can be useful for debugging. yarn test will now throw an error and prevent tests if use a console.log. Instead, you'd have to use yarn jest src/ which is a bit more verbose.

Few workarounds for this

• "test": "yarn lint && jest src/.*", can be "test": "jest src/.* && yarn lint", so tests run before linting.
• Users can run yarn ci (Which isn't intuitive)
• We lint in precommit anyway, so do we really need lint in test?

.jison files aren't scanned by ESLint, so we still need to manually check those files.

I think we should have a pipeline which converts .jison to .js and then runs eslint? It should be fairly straightforward as we're already converting jison to js here and here.

Errors caught with manual checks usually slip through the cracks after a while and take up our time reviewing PRs.

[aloisklink]

• Users can run yarn ci (Which isn't intuitive)
• We lint in precommit anyway, so do we really need lint in test?

My gut feeling is to remove the lint from the yarn test command, since it's covered by pre-commit/husky anyway (and the https://github.com/mermaid-js/mermaid/actions/workflows/lint.yml CI action). The yarn ci could also be removed then, since it'll be exactly the same as yarn test.

If you like the idea, I'm happy to make another PR for that!

I think we should have a pipeline which converts .jison to .js and then runs eslint? It should be fairly straightforward as we're already converting jison to js here and here.

Errors caught with manual checks usually slip through the cracks after a while and take up our time reviewing PRs.

I had a quick try with running eslint on generated jison files, but it looks like the ESLint can't understand the jison generated files:

alois@me: ~/Documents/mermaid (other/3339_forbid-console-in-src-code)$ yarn jison ./src/diagrams/c4/parser/c4Diagram.jison && yarn eslint ./c4Diagram.js
yarn run v1.22.15
$ /home/alois/Documents/mermaid/node_modules/.bin/eslint ./c4Diagram.js

/home/alois/Documents/mermaid/c4Diagram.js
  909:15  error  Parsing error: Missing semicolon. (909:15)

✖ 1 problem (1 error, 0 warnings)

Instead, what I've done in b67657a is to use the library jest-fail-on-console, which fails jest tests that run console.log, see example screenshot below:

It's still not great, since it relies on jest tests covering everything in the .jison file, but it should hopefully cover mostly everything.

[sidharthv96]

I had a quick try with running eslint on generated jison files, but it looks like the ESLint can't understand the jison generated files:

yarn eslint --no-eslintrc --rule no-console:error --parser "@babel/eslint-parser" ./c4Diagram.js

checks for console only and parses the file properly.

Jest might not cover all the branches, so let's stick with eslint. (which is another problem we should fix - Coverage reports).

[aloisklink]

checks for console only and parses the file properly.

Jest might not cover all the branches, so let's stick with eslint. (which is another problem we should fix - Coverage reports).

Awesome! Good catch. I've added some ESLint tests for the .jison files using GitHub Actions in 22d20cc (and removed my old jest-fail-on-console commit).

It's a bit hacky (it's some basic bash code), since it only runs in CI, but I feel like it's a bit too much effort to make a custom eslint-plugin-jison to get jison parsing working when running yarn lint.

An example failing run: https://github.com/aloisklink/mermaid/runs/8198619955

which is another problem we should fix - Coverage reports

That would be cool! Although again, I feel like coverage reports and .jison files might not mix well.

[sidharthv96]

It's a bit hacky (it's some basic bash code), since it only runs in CI, but I feel like it's a bit too much effort to make a custom eslint-plugin-jison to get jison parsing working when running yarn lint.

Totally agree. The current approach is fine!

I feel like coverage reports and .jison files might not mix well.

I don't think it will either, I was thinking of the JS & TS file coverage.

[aloisklink]

I was thinking of the JS & TS file coverage.

👍 It would be great!

Force pushed to 6e81ee9, since I accidentally created the wrong indentation when trying to fix merge conflicts with GitHub's UI.