[pull] develop from mermaid-js:develop - #229
Merged
Merged
Conversation
getBuildConfig built the core build's `external` list from `dependencies`
only. mermaid is a *peer* dependency of the layout plugins, so it was not
external, and a runtime (non-type) import of it resolved through the
package `exports` to dist/mermaid.core.mjs — which esbuild then inlined
wholesale.
That stayed invisible while mermaid-layout-elk imported mermaid type-only
(erased at compile time). Once src/render.ts began importing values
(`import mermaid, { createCommonLayoutRenderer } from 'mermaid'`), the
latent misconfiguration turned on: the published core entry went from
3 files / ~36 KB to 106 files / 6.6 MB, bundling every diagram renderer
plus katex and a second, version-skewed mermaid.
The duplicate matters beyond size. Module-level singletons are duplicated
with it, so the plugin renders against its own inlined mermaid rather than
the host's — meaning a mermaid rendering fix does not reach the ELK layout
path until layout-elk is itself rebuilt and republished. That is how the
`edgePaths` -> `edgePath` edge-container rename (#8124) stayed broken for
ELK consumers after the mermaid-side fix.
Push peerDependencies into `external` alongside dependencies. Scope is
narrow: only the `if (core)` branch changes, so the self-contained `esm`
entry still inlines mermaid for standalone and dev-server use, and
mermaid itself declares no peerDependencies, so its own build is
untouched. Verified: elk's core entry returns to 3 files / ~41 KB with
mermaid as a bare external, and a consumer bundle drops from two copies
of mermaid to one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… colour themes Until now only ER, sequence, git and requirement diagrams read the colour themes' `borderColorArray` / `bkgColorArray`. Class boxes and flowchart subgraph containers rendered in a single uniform colour under `redux-color` and `redux-dark-color`. Class diagrams: each class gets its own border and fill from the palette, cycling every 12, exactly as ER entities do. A class box is the structural twin of an ER entity -- a titled box with member rows naming one distinct participant -- so the same index-based palette applies. Namespaces and notes stay outside the cycle: a namespace is a container, and a note carries the theme's fixed note colour. Flowchart subgraphs: each container gets its own border and fill. The nodes inside are deliberately left uniform -- a flowchart node is a step in a flow rather than a distinct participant, and node colour is already how `classDef` / `style` convey meaning. `clusters.js` is shared by every diagram that has subgraphs, so it stamps `data-color-id` unconditionally and each diagram opts in by emitting the matching rules in its own stylesheet. State, block and class-namespace containers get an inert attribute and no colour. Subgraph colours key off the declaration index rather than a running counter: `getData()` walks subgraphs in reverse and skips ones hidden inside a collapsed ancestor, so a counter would hand out colours in reverse reading order and reshuffle them whenever a subgraph was collapsed. Explicit user styling still wins. `classDef` / `style` declarations are applied as inline `style` attributes and none of the new rules are `!important`, so `style MySubgraph fill:#00ff00` keeps painting the container green -- there is a test for that.
Extract the shared gate. `COLOR_THEMES` existed in five copies and the stamping block was duplicated verbatim between `clusters.js` and `classBox.ts`; two idioms for the same gate had already diverged (`er/styles.ts` keys off the theme name, `requirement/styles.js` off the array being non-empty). New `diagrams/common/colorThemeGate.ts` owns `COLOR_THEMES`, `isColorTheme`, `hasPalette`, `colorSlotCount`, `safeLook` and `stampColorSlot`; the four call sites import it. The spec was already sitting at the path that implied this module. Harden `look` before it reaches a CSS selector. `look` is a top-level config key, so it is settable from diagram text, and `config.sanitize` only drops values containing `<`, `>` or `url(data:` -- braces and quotes survive, which is enough to close the attribute selector early and escape the `#svgId` scoping stylis applies. Confirmed by probe: the value lands verbatim in the emitted rule. `safeLook` rejects anything that is not a bare word. NOTE: this pattern is pre-existing in `er/styles.ts` and `requirement/styles.js` on develop, so it affects released versions; those are deliberately left for a coordinated fix and can now import `safeLook` rather than adding a third copy. Close the collapsedGroup seam. A collapsed subgraph rendered uncoloured beside tinted siblings. It renders as `.node .collapsed-group` rather than `.cluster`, so the flowchart stylesheet gained that selector; its own colours are presentation attributes, which the rules outrank while still losing to user inline style. Verified by render: First/Second(collapsed)/Third take slots 0/1/2, so collapsing does not reshuffle siblings. Cover the join. The unit tests checked each end -- the db hands out slots, the stylesheet emits rules -- but not that the stamped attribute meets the emitted selector on a real element. Added the two colour themes to `classDiagram-neo.spec.js` (42 tests) and a new `flowchart-redux-color-subgraphs.spec.ts` (16) covering nesting, collapse and user-styled precedence. Also: derive `PLAIN_THEMES` from the theme registry so a new theme is covered automatically; use `colorSlotCount` in both stylesheets so they agree when `THEME_COLOR_LIMIT` is absent; `hasPalette` uses `Array.isArray` so a string cannot pass as a palette; reconcile the `clusters.js` comment with the code, which is conditional on the theme rather than unconditional. Changeset shortened.
Review feedback: cut to one paragraph, matching the norm of the existing changesets on develop.
…se two seams
The namespace test could not fail. `addClassesToNamespace` early-returns when the
namespace does not exist, and the test never called `addNamespace` -- so no
namespace node was created and the assertion passed on absence rather than on
behaviour. Verified the reviewer's check: giving namespaces `colorIndex: 99`, the
exact thing the test forbids, left all three passing. It now creates the
namespace and attaches a class, and fails on that regression.
Subgraph colour slots did not follow source order for nested subgraphs, contrary
to what the code, the test name and the description all claimed. `addSubGraph`
runs when a subgraph *closes*, so `subGraphs` holds nested ones before their
parent: `Outer { InnerOne, InnerTwo }, Sibling` arrives as [InnerOne, InnerTwo,
Outer, Sibling], and taking the array index handed Outer slot 2 while its own
children took 0 and 1. A pre-order walk of the containment forest recovers source
order, assigned once so the collapsed and expanded branches cannot drift.
The existing test could not have caught it -- three flat subgraphs, where close
order and source order are identical. Replaced with a nested fixture, and
confirmed it is the only test in the file that fails against the old
`colorIndex: i`.
Collapsed markers kept their default colour. The palette reached
`.collapsed-group` but not `.collapsed-indicator` or `.collapsed-separator`,
which take `clusterBorder` further down -- so a collapsed subgraph rendered a
tinted container with default-coloured dots and separator, the same seam one
level down from the one this PR set out to close.
Those selectors also now cover `rough-node`. `collapsedGroup.ts` goes through
`getNodeClasses`, which returns `rough-node` rather than `node` under the
handDrawn look, so a `.node`-only selector left handDrawn collapsed containers
uncoloured. Each descendant is appended to both prefixes separately: writing
`SLOT.node, SLOT.rough-node .thing` would attach the descendant to the last item
of the list only and silently match nothing under the classic look. Checked the
emitted CSS rather than assuming. Clusters were never affected -- `clusters.js`
sets the `cluster` class directly.
Corrected `colorSlotCount`'s docstring: it claimed to clamp to what the palette
can supply, but it never sees a palette -- it only floors a non-numeric
THEME_COLOR_LIMIT. The clamping is `stampColorSlot`'s, via `% palette.length`.
`colorSlotCount` returned `THEME_COLOR_LIMIT`, but `stampColorSlot` wraps at `palette.length`. The two disagree whenever a palette is longer than the limit: its tail gets stamped `color-N` with no rule emitted for that slot, and those items render uncoloured beside their neighbours. Confirmed with a limit of 3 and a five-entry palette -- the stylesheet emitted slots 0..2 while the fifth item was stamped `color-4`. The count now covers the palette as well as the limit, so every slot that can be stamped has a rule. A shorter palette still emits the full limit and cycles, as before. No change for either shipped colour theme: both carry exactly THEME_COLOR_LIMIT entries, so the two counts already agreed -- which is what hid this. It takes a `themeVariables` override to reach, the same shape as the length mismatches this PR's shared gate exists to prevent. The new assertions name the slots that lost their rule rather than counting them, and were checked against the old `return limit` -- three fail, listing slots 3..19.
The stylesheets pass this straight into a `for` condition, so it has to be a value a loop can finish on. `typeof x === 'number' && x > 0` was not: `Infinity` satisfies both, and the class and flowchart generators then loop forever instead of rendering something merely wrong. Reachable from diagram text, not only from site config, which is worse than the review put it: `THEME_COLOR_LIMIT: .inf` in front matter parses to `Infinity` under the `JSON_SCHEMA` mermaid loads YAML with -- confirmed against the pinned js-yaml. A large finite integer such as `1e9` wedges generation just as effectively while passing every previous check. Now accepts only a positive integer up to `MAX_COLOR_SLOTS`, and falls back to the default otherwise. The cap bounds the loop rather than expressing a design limit -- every shipped palette has twelve entries, so 64 is generous. Verified in both directions: the pre-fix bound for `Infinity` is `Infinity` and a loop on it was still running after 121 million iterations, while class stylesheet generation with `THEME_COLOR_LIMIT: Infinity` now terminates and emits exactly twelve slots.
Three rounds of review have now found the same bug in different clothes, because two places decided the slot space independently: `stampColorSlot` assigns `colorIndex % palette.length`, while the stylesheets emitted `0 .. limit - 1`. Every fix kept both deciders and tried to keep them lined up. - a limit longer than the palette: tail stamped, no rule emitted - a palette longer than the limit (fe5fec1): same, other direction - a palette longer than MAX_COLOR_SLOTS: the cap I added to stop `Infinity` hanging the loop reintroduced it a third time There is only one correct count, and it is not a policy choice. `stampColorSlot` can produce exactly the ids `0 .. palette.length - 1` -- verified by enumerating 500 indices: a two-entry palette yields only slots 0 and 1, a seventy-entry palette reaches slot 69. So the count is `palette.length`; anything less leaves items stamped with no rule, anything more is dead CSS. `paletteSlotCount` is now the single source of truth and both sides read it, which makes the two impossible to disagree rather than something a bound has to police. `THEME_COLOR_LIMIT` still governs callers that emit slots without stamping -- timeline numbers `.section-N` classes rather than palette slots -- and stays bounded there, because a loop runs on it directly and `Infinity` is reachable from front matter. `MAX_COLOR_SLOTS` now applies only to that path. The missing test is the point. Nothing asserted the two sides agree, only that each behaved as its author expected, which is why each round passed its own tests and failed the next review. `emitted slots and stampable slots agree` compares the two sets across palette lengths from 1 to MAX+50 and limits including `Infinity` and `undefined`, per stylesheet as well as per function. Replaying all three historical forms against it: 21, 13 and 10 assertions fail respectively. Two of the corrected expectations were fe5fec1's -- "still emits the full limit for a palette shorter than it" and "keeps the limit when the palette is shorter, so the cycle still repeats". Both are wrong on the merits: with a two-entry palette, slots 2..11 can never be stamped, so those were rules nothing could match. The cycling happens in `stampColorSlot`, not in the rule count. One of the corrected expectations was my own cap, which was the bug this round found.
…wchart-palette feat(themes): colour class boxes and flowchart subgraphs in the redux colour themes
…-core-build fix(build): externalize peerDependencies in core builds
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )