From 6312774cdac89be2cbda1734dad1c4ed68eaba75 Mon Sep 17 00:00:00 2001 From: Michael Naumov Date: Wed, 2 Sep 2026 12:24:44 -0600 Subject: [PATCH 1/2] fix(no-nodejs-modules): take the desktop-only signal from an option, not only a manifest A repo that is not a plugin -- a library, tooling, a standalone CLI alongside the plugin -- currently has to ship a fake manifest.json to use this config without noise. The rule was configured as `manifest && manifest.isDesktopOnly ? "off" : "warn"`, so the only way to tell it that Node APIs are available was a manifest with `isDesktopOnly: true`. It now takes an `isDesktopOnly` option, defaulting to that same manifest value, and the config sets a plain "warn". Behaviour for an existing plugin repo is identical; a repo with no manifest can say `["warn", { isDesktopOnly: true }]` instead of inventing one. This matters because the rule is listed in eslint-comments/no-restricted-disable, so a disable comment is not an available escape. `defaultOptions` had to become `[{}]` rather than `[]`: applyDefault iterates the defaults array, so an empty one silently discards the user's options -- the option had no effect at all until that changed. Worth checking wherever else a rule takes options with an empty default. The other half of the same problem is in `getManifest()`, which is why it is in this commit rather than its own: it caught the ENOENT from a missing manifest.json, printed "Failed to load JSON file: " to console.error, and returned null, so a repo with no manifest got that dump on every lint run. Its absence is not an error and is now checked for first. A manifest that exists but does not parse is a real defect and still reports. The node globals half of the manifest read is unchanged -- a repo that wants them can add `globals.node` to its own config in one line, which does not seem worth new API surface. Closes #178. This touches getManifest(), which #107 also edits to walk up the directory tree; the two should combine cleanly but will conflict textually. Docs. configuration.md gains a "Code that only runs on desktop" section under "Disabling rules for specific files": it shows the option, notes that a desktop-only plugin still needs no configuration because the default reads the manifest, and says why the option exists at all -- the no-manifest case. Nothing in the README changes, since the default behaviour is unchanged. --- docs/configuration.md | 21 ++++++++++++++++++++ docs/rules/no-nodejs-modules.md | 26 ++++++++++++++++++++++++ lib/index.ts | 5 +++-- lib/manifest.ts | 10 ++++++++++ lib/rules/noNodejsModules.ts | 35 +++++++++++++++++++++++++++++---- tests/noNodejsModules.test.ts | 18 +++++++++++++++++ 6 files changed, 109 insertions(+), 6 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index f6322d3..d21399e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -167,6 +167,27 @@ export default defineConfig([ ]); ``` +### Code that only runs on desktop + +`no-nodejs-modules` reports because Node APIs are unavailable on mobile. Where that does not apply — +a desktop-only plugin, or code that never runs inside Obsidian at all — say so with the rule's +option rather than switching it off, so guarded and unguarded imports are still distinguished +everywhere else: + +```js +{ + rules: { + "obsidianmd/no-nodejs-modules": ["warn", { isDesktopOnly: true }], + }, +} +``` + +The option defaults to the `isDesktopOnly` value of a `manifest.json` in the working directory, so a +desktop-only plugin needs no configuration at all. It exists for the case where there is no manifest +to read it from — a library, tooling, or a standalone CLI in the same repo — which previously meant +adding a `manifest.json` that served no other purpose. Note that this rule is listed in +`eslint-comments/no-restricted-disable`, so a disable comment is not an available escape. + ESLint does not support disabling all rules from a plugin with a single glob like `"obsidianmd/*": "off"`. You can set each rule to `"off"` individually, or use `ruleConfigs` to generate the overrides programmatically: ```js diff --git a/docs/rules/no-nodejs-modules.md b/docs/rules/no-nodejs-modules.md index 6d29bff..0a940ea 100644 --- a/docs/rules/no-nodejs-modules.md +++ b/docs/rules/no-nodejs-modules.md @@ -5,3 +5,29 @@ ⚠️ This rule _warns_ in the following configs: ✅ `recommended`, 🇬🇧 `recommendedWithLocalesEn`. + +## Options + + + +| Name | Description | Type | +| :-------------- | :------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | :------ | +| `isDesktopOnly` | Whether the code being linted only ever runs on desktop, which makes Node.js APIs available. Defaults to the isDesktopOnly value of a manifest.json in the working directory, so a repo with no manifest -- a library, tooling, a standalone CLI -- can say so without shipping a fake one. | Boolean | + + + +`isDesktopOnly` defaults to the `isDesktopOnly` value of a `manifest.json` in the +working directory. A repo that is not a plugin — a library, tooling, a +standalone CLI in the same repo — has no manifest to carry that flag, and used to +need a fake one purely to keep this rule quiet. Set the option instead: + +```js +{ + rules: { + "obsidianmd/no-nodejs-modules": ["warn", { isDesktopOnly: true }], + }, +} +``` + +This rule is listed in `eslint-comments/no-restricted-disable`, so a disable +comment is not an available escape. diff --git a/lib/index.ts b/lib/index.ts index 1ff90f4..f10b2fa 100644 --- a/lib/index.ts +++ b/lib/index.ts @@ -212,8 +212,9 @@ const flatRecommendedGeneralRules: RulesConfig = { ], "@microsoft/sdl/no-document-write": "warn", "@microsoft/sdl/no-inner-html": "warn", - "obsidianmd/no-nodejs-modules": - manifest && manifest.isDesktopOnly ? "off" : "warn", + // The rule resolves the desktop-only signal itself, defaulting to the + // manifest, so the severity no longer has to be computed here. + "obsidianmd/no-nodejs-modules": "warn", "import/no-extraneous-dependencies": "warn", "obsidianmd/rule-custom-message": [ "error", diff --git a/lib/manifest.ts b/lib/manifest.ts index dcf457f..9700491 100644 --- a/lib/manifest.ts +++ b/lib/manifest.ts @@ -8,6 +8,16 @@ export function getManifest(): PluginManifest | null { return cachedManifest; } + // A library, a tooling repo or a standalone CLI legitimately has no + // manifest, so its absence is not an error and must not print anything -- + // it used to write to console.error on every lint run in such a repo. A + // manifest that exists but does not parse is a real defect and still + // reports. + if (!fs.existsSync("manifest.json")) { + cachedManifest = null; + return cachedManifest; + } + try { const data = fs.readFileSync("manifest.json", "utf8"); cachedManifest = JSON.parse(data); diff --git a/lib/rules/noNodejsModules.ts b/lib/rules/noNodejsModules.ts index ca6cef0..74e9513 100644 --- a/lib/rules/noNodejsModules.ts +++ b/lib/rules/noNodejsModules.ts @@ -1,23 +1,50 @@ import { AST_NODE_TYPES } from "@typescript-eslint/utils"; import type { TSESTree } from "@typescript-eslint/utils"; import { isBuiltin } from "node:module"; +import { getManifest } from "../manifest.js"; import { docsUrl, ruleCreator } from "../ruleCreator.js"; -export default ruleCreator({ +interface NoNodejsModulesOptions { + isDesktopOnly?: boolean; +} + +export default ruleCreator<[NoNodejsModulesOptions?], "noNodejs">({ meta: { type: "problem", docs: { description: "Disallow importing Node.js built-in modules unless guarded by Platform.isDesktop", url: docsUrl("no-nodejs-modules"), }, - schema: [], + schema: [ + { + type: "object", + properties: { + isDesktopOnly: { + type: "boolean", + description: + "Whether the code being linted only ever runs on desktop, which makes Node.js APIs available. Defaults to the isDesktopOnly value of a manifest.json in the working directory, so a repo with no manifest -- a library, tooling, a standalone CLI -- can say so without shipping a fake one.", + }, + }, + additionalProperties: false, + }, + ], messages: { noNodejs: "Do not import Node.js built-in module \"{{module}}\". Node.js APIs are not available on mobile. Use a dynamic import() or require() guarded by Platform.isDesktop instead.", }, }, - defaultOptions: [], - create(context) { + // Must be [{}] rather than []: applyDefault iterates the defaults array, so + // an empty one discards the user's options instead of merging them. + defaultOptions: [{}], + create(context, [options]) { + // The rule is listed in eslint-comments/no-restricted-disable, so a + // disable comment is not an available escape; this option is. + const isDesktopOnly = + options?.isDesktopOnly ?? getManifest()?.isDesktopOnly ?? false; + if (isDesktopOnly) { + return {}; + } + return { // Static import declarations can never be guarded at runtime ImportDeclaration(node: TSESTree.ImportDeclaration) { diff --git a/tests/noNodejsModules.test.ts b/tests/noNodejsModules.test.ts index 06407ef..1cd2863 100644 --- a/tests/noNodejsModules.test.ts +++ b/tests/noNodejsModules.test.ts @@ -17,6 +17,18 @@ ruleTester.run("no-nodejs-modules", noNodejsModules, { name: "importing a third-party module is allowed", code: "import _ from 'lodash';", }, + { + // A library, tooling repo or standalone CLI has no manifest to + // carry isDesktopOnly, and used to need a fake one to say so. + name: "static import is allowed when isDesktopOnly is set", + code: "import fs from 'node:fs';", + options: [{ isDesktopOnly: true }], + }, + { + name: "unguarded dynamic import() is allowed when isDesktopOnly is set", + code: "const fs = await import('fs');", + options: [{ isDesktopOnly: true }], + }, { name: "dynamic import() inside if (Platform.isDesktop) is allowed", code: "if (Platform.isDesktop) { const fs = await import('fs'); }", @@ -262,6 +274,12 @@ ruleTester.run("no-nodejs-modules", noNodejsModules, { `, errors: [{ messageId: "noNodejs" }], }, + { + name: "explicitly setting isDesktopOnly false still reports", + code: "import fs from 'node:fs';", + options: [{ isDesktopOnly: false }], + errors: [{ messageId: "noNodejs" }], + }, { name: "dynamic import() with Platform check but no early exit is forbidden", code: ` From 1494c1d943b350ca901fbd7d7ab52308cffcb873 Mon Sep 17 00:00:00 2001 From: Michael Naumov Date: Wed, 23 Sep 2026 18:35:30 -0600 Subject: [PATCH 2/2] fix(no-nodejs-modules): count only a literal true as a desktop-only manifest A manifest.json belongs to the repo being linted -- in the community scanner, to whoever submitted the plugin -- and reaches the rule through JSON.parse, so its isDesktopOnly is a boolean only to TypeScript. The rule tested it for truthiness, so "isDesktopOnly": "false" -- a string, which validate-manifest reports as a wrong type, but only as a warning and only after this rule has already read it -- returned the empty visitor, and the rule reported nothing at all for that repo. The same holds for 1, "no", [] and {}: a plugin could turn the Node.js-import check off for itself with one malformed manifest field. isManifestDesktopOnly() in lib/manifest.ts is now the single place that decision is made. It takes the manifest as a parameter so a test can hand it a malformed one: getManifest() caches at module level and reads manifest.json from the process cwd, so a RuleTester case cannot reach a manifest of its own choosing. Verified end to end against the built plugin, with a real ESLint run in a directory whose manifest.json carries "isDesktopOnly": "false". Before the change an unguarded `import fs from "node:fs"` there produced no output whatever; after it, the rule reports. Two more reads of the same field still test it for truthiness -- regex-lookbehind and the node-globals block of the recommended config. Both are on master rather than on this branch, so they are left for their own change. --- docs/rules/no-nodejs-modules.md | 5 ++++ lib/manifest.ts | 15 +++++++++++ lib/rules/noNodejsModules.ts | 4 +-- tests/manifest.test.ts | 46 +++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 tests/manifest.test.ts diff --git a/docs/rules/no-nodejs-modules.md b/docs/rules/no-nodejs-modules.md index 0a940ea..d672ecd 100644 --- a/docs/rules/no-nodejs-modules.md +++ b/docs/rules/no-nodejs-modules.md @@ -29,5 +29,10 @@ need a fake one purely to keep this rule quiet. Set the option instead: } ``` +Only a literal `true` in that manifest counts. The file belongs to the repo +being linted, so its `isDesktopOnly` is whatever JSON it holds: a non-boolean +value -- `"false"`, `1`, `[]` -- is read as *not* desktop-only and the rule +keeps reporting, rather than being turned off by a malformed field. + This rule is listed in `eslint-comments/no-restricted-disable`, so a disable comment is not an available escape. diff --git a/lib/manifest.ts b/lib/manifest.ts index 9700491..820b322 100644 --- a/lib/manifest.ts +++ b/lib/manifest.ts @@ -28,3 +28,18 @@ export function getManifest(): PluginManifest | null { return cachedManifest; } } + +/** + * Whether a manifest says its plugin is desktop-only. + * + * A manifest.json belongs to the repo being linted, so `isDesktopOnly` is a + * `boolean` only to TypeScript -- at runtime it is whatever the file holds. + * `"false"`, `1`, `[]` and `{}` are all truthy, so a truthiness test lets a + * malformed field turn a rule off for the very repo that shipped it. Only a + * real `true` counts. + */ +export function isManifestDesktopOnly( + manifest: PluginManifest | null = getManifest(), +): boolean { + return (manifest as { isDesktopOnly?: unknown } | null)?.isDesktopOnly === true; +} diff --git a/lib/rules/noNodejsModules.ts b/lib/rules/noNodejsModules.ts index 74e9513..2b3dcb1 100644 --- a/lib/rules/noNodejsModules.ts +++ b/lib/rules/noNodejsModules.ts @@ -1,7 +1,7 @@ import { AST_NODE_TYPES } from "@typescript-eslint/utils"; import type { TSESTree } from "@typescript-eslint/utils"; import { isBuiltin } from "node:module"; -import { getManifest } from "../manifest.js"; +import { isManifestDesktopOnly } from "../manifest.js"; import { docsUrl, ruleCreator } from "../ruleCreator.js"; interface NoNodejsModulesOptions { @@ -40,7 +40,7 @@ export default ruleCreator<[NoNodejsModulesOptions?], "noNodejs">({ // The rule is listed in eslint-comments/no-restricted-disable, so a // disable comment is not an available escape; this option is. const isDesktopOnly = - options?.isDesktopOnly ?? getManifest()?.isDesktopOnly ?? false; + options?.isDesktopOnly ?? isManifestDesktopOnly(); if (isDesktopOnly) { return {}; } diff --git a/tests/manifest.test.ts b/tests/manifest.test.ts new file mode 100644 index 0000000..5b390f5 --- /dev/null +++ b/tests/manifest.test.ts @@ -0,0 +1,46 @@ +import assert from "node:assert"; +import { describe, it } from "mocha"; +import type { PluginManifest } from "../types/manifest.js"; +import { isManifestDesktopOnly } from "../lib/manifest.js"; + +// A manifest.json is supplied by the repo being linted -- in the community +// scanner, by whoever submitted the plugin -- and reaches these rules through +// JSON.parse, so isDesktopOnly is a boolean only to TypeScript. Every value +// below is truthy at runtime, and a truthiness test would read each of them as +// desktop-only, which turns no-nodejs-modules off for that repo. +const NOT_DESKTOP_ONLY: unknown[] = [ + "false", + "true", + "no", + 1, + [], + {}, + false, + 0, + null, + undefined, +]; + +function manifestWith(isDesktopOnly: unknown): PluginManifest { + return { isDesktopOnly } as unknown as PluginManifest; +} + +describe("isManifestDesktopOnly", () => { + it("is true only for a real boolean true", () => { + assert.strictEqual(isManifestDesktopOnly(manifestWith(true)), true); + }); + + for (const value of NOT_DESKTOP_ONLY) { + it(`is false for ${JSON.stringify(value) ?? String(value)}`, () => { + assert.strictEqual(isManifestDesktopOnly(manifestWith(value)), false); + }); + } + + it("is false for a manifest with no isDesktopOnly at all", () => { + assert.strictEqual(isManifestDesktopOnly({} as PluginManifest), false); + }); + + it("is false when there is no manifest", () => { + assert.strictEqual(isManifestDesktopOnly(null), false); + }); +});