Repository navigation
build: lint for WebKit 15.4 with eslint-plugin-compat - #8212
Conversation
Lint the code that ships in the bundles (`packages/*/src`), with `compat/compat`. Safari 15.4 is the floor that we're not officially supporting in Mermaid v11, but is one we're trying to support as a rough guideline.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/12.0.0 #8212 +/- ##
==================================================
+ Coverage 80.13% 80.16% +0.02%
==================================================
Files 624 624
Lines 85311 85411 +100
Branches 16183 16103 -80
==================================================
+ Hits 68366 68471 +105
+ Misses 15909 15906 -3
+ Partials 1036 1034 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…slint-plugin-compat Fixes the failing unit tests.
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
ashishjain0512
left a comment
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Nice, tightly-scoped change — a machine-checked support floor beats a tribal-knowledge one, and it's a good sign that zero source changes were needed to land it at safari >= 15.4.
I checked out the PR head (4cd8a1d), installed from the lockfile, and verified the mechanism rather than just reading the diff. Notes below.
What's working well
🎉 The target genuinely resolves. The one thing I wanted to disprove was a silent fallback to browserslist defaults, since packages/mermaid/package.json has no browserslist field of its own and eslint-plugin-compat resolves from the linted file's path (browserslistDir = context.filename, then browserslist(undefined, { path })). findConfig() walks straight past the package-level package.json and lands on the root one:
findConfig(packages/mermaid/src/mermaid.ts) -> {"defaults":["safari >= 15.4"]}
floor of resolved targets -> safari 15.4
And end to end, on a scratch file under packages/mermaid/src:
error Array.toSorted() is not supported in Safari 15.4 compat/compat
with structuredClone() (Safari 15.4) correctly not flagged. So the rule is live and calibrated to the right floor.
🎉 The files/ignores scoping does what the comment says. Via eslint --print-config:
| file | compat/compat |
|---|---|
packages/mermaid/src/mermaid.ts |
on |
packages/mermaid/src/diagrams/class/styles.js |
on |
packages/mermaid/src/diagrams/treemap/utils.spec.ts |
off |
packages/mermaid/src/docs/.vitepress/canonical-config.ts |
off |
🎉 Generated code is already out of scope — the global ignores block (eslint.config.js:19-29) covers **/generated/ and packages/mermaid/src/config.type.ts, so the Langium output can't start failing a rule whose fix would have to live in a generator.
🎉 lintAllEsApis: true won't be silently undone. v7 computes it as settings.lintAllEsApis === true || (!polyfills.includes('es:all') && !isUsingTranspiler(file)), and isUsingTranspiler find-ups for a babel config / babel key — of which there are none here. The explicit true short-circuits it either way. Good call setting it, since lintAllEsApis is the half that catches toSorted at all.
🎉 Clean dependency footprint — devDependencies only, nothing under packages/*/src imports it, none of the four new packages has a pre/post-install script, and the lockfile diff is +42/−0 with no existing resolution touched (so dompurify and friends are provably unchanged).
Things to address
🟡 [important] The rule's API knowledge is ~2 years stale, so green lint isn't a guarantee. From the same scratch file, these were not flagged:
Object.groupBy([1, 2, 3], (n) => String(n % 2)); // Safari 17.4
Promise.withResolvers(); // Safari 17.4while toSorted (Safari 16.4) was. The cause looks like the dependency split visible in the lockfile: the plugin ships @mdn/browser-compat-data@6.1.5, but ast-metadata-inferer@0.8.1 — which generates the AST metadata that decides what the rule can even recognise — pins @mdn/browser-compat-data@5.7.6. Anything added to BCD after 5.7.6 is invisible to compat/compat.
Worth noting explicitly (in the PR body, or as a line in the eslint.config.js comment) that this is a best-effort net rather than a gate — otherwise the next person reads a passing lint as proof. Related: the weekly Update Browserslist workflow refreshes caniuse-lite, which affects how safari >= 15.4 expands but not what APIs the rule knows about; keeping that current means bumping the plugin.
🟢 [nit] **/*.test.ts isn't ignored — eslint.config.js:169 ignores **/*.spec.{ts,js} only, so five files that never ship are now linted against the browser floor:
packages/mermaid-layout-tidy-tree/src/layout.test.tspackages/mermaid/src/diagrams/mindmap/mindmapDb.getData.test.tspackages/mermaid/src/diagrams/treemap/utils.test.tspackages/mermaid/src/rendering-util/layout-algorithms/cose-bilkent/cytoscape-setup.test.tspackages/mermaid/src/rendering-util/layout-algorithms/cose-bilkent/layout.test.ts
Harmless today (nothing in them trips the rule), but it's a latent false positive for whoever reaches for toSorted in a Vitest-only file. '**/*.{spec,test}.{ts,js}' covers both. The rest of the config has the same *.spec.*-only assumption (:250, :257), so this is a pre-existing habit rather than something you introduced — it just bites harder here, where the rule is specifically about shipped code.
🟢 [nit] Two of the four ignore entries are inert. **/vitepress/** doesn't match the directory, which is .vitepress (a literal path segment has to match exactly — the leading dot matters), and **/scripts/** only ever had packages/mermaid/src/docs/.vitepress/scripts/ to match, which **/docs/** already covers. Confirmed by the print-config table above: .vitepress/canonical-config.ts is excluded by **/docs/**, not by the pattern that looks like it's doing the work. ignores: ['**/*.{spec,test}.{ts,js}', '**/docs/**'] is the honest version.
Suggestions / questions
💡 The syntax half of the floor is still unenforced. compat/compat checks API availability; nothing checks emitted syntax. .esbuild/util.ts sets no target, so the shipped bundles are built at esbuild's default (esnext) — meanwhile the dev server hardcodes target: 'es2020' (.esbuild/server.ts:240) and the docs build already pins target: 'modules' with a comment about Vite 7 moving its default (packages/mermaid/src/docs/vite.config.ts:21-22). Three different notions of "old enough", now with a fourth (browserslist) that only lint reads. Deriving the esbuild target from the same browserslist field would make the floor mean one thing. Also out of reach either way: bundled dependencies (d3, dagre, …) aren't linted, so their API use isn't held to 15.4. Both feel like follow-ups rather than this PR's job — happy for this to land as-is.
💡 Since this targets release/12.0.0, is the plan to pick this up on develop via the eventual forward-merge, or would a matching PR there be worth it so the rule doesn't quietly vanish from v12+ development in the meantime?
Security
No XSS or injection issues identified. No source files are touched, so there's no DOM sink, SVG emission, or DOMPurify call site in scope. I did check the one plausible indirect path — a new root browserslist field silently retargeting a transpiler and changing shipped output — and it's a non-issue: there is no autoprefixer, postcss, @babel/preset-env, or @vitejs/plugin-legacy anywhere in the repo, no .browserslistrc, and no package-level browserslist, so the field is read by eslint-plugin-compat at lint time and nothing else. Bundled output is unchanged.
Verdict: comment only, nothing blocking. The two nits are one-liners in eslint.config.js:169; the 🟡 is a documentation ask, not a code change.
`**/vitepress/**` and `**/scripts/**` are unnecessary, since the only thing they affect are in `packages/mermaid/src/docs`, which `**/docs/**` already handles. I've also ignored `**/__mocks__/**` since that is currently included, but is only needed for vitest.
These are caught by TypeScript on
I think it's better to fix this in a separate PR by renaming all these test files to
Fixed in b0ccaf8
TypeScript enforces most of this. #8213 will update ESBuild. And I'm working on a PR that will add a Safari 17.4 E2E test.
I'll leave this up to @ashishjain0512 to decide, but we'll probably merge |
2 participants
📑 Summary
Note
This PR targets the
release/12.0.0branch since that has already branched off ofdevelop.Lint the code that ships in the bundles (
packages/*/src), withcompat/compat.📏 Design Decisions
Safari 15.4 is the floor that we're not officially supporting in Mermaid v11 (hence why it's not documented anywhere), but is one we're trying to support as a rough guideline.
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.