From 16ac217ed0243e367717c558795548a8a4d49ff4 Mon Sep 17 00:00:00 2001 From: Gordon Smith Date: Fri, 27 Jun 2025 13:23:55 +0100 Subject: [PATCH] fix: Improve type-leaks test Was missing some node deps still in devDeps Signed-off-by: Gordon Smith --- packages/comms/package.json | 10 +- packages/comms/src/clienttools/eclMeta.ts | 4 +- packages/comms/src/clienttools/eclcc.ts | 8 +- packages/comms/src/index.node.ts | 2 +- packages/markdown-it-plugins/package.json | 6 +- tests/type-leaks/type-dependencies.spec.ts | 295 ++++++++++++++++----- 6 files changed, 243 insertions(+), 82 deletions(-) diff --git a/packages/comms/package.json b/packages/comms/package.json index 6fd2e3b9d2..3529600f6a 100644 --- a/packages/comms/package.json +++ b/packages/comms/package.json @@ -75,7 +75,12 @@ "@hpcc-js/util": "^3.3.2", "@xmldom/xmldom": "0.9.8", "abort-controller": "3.0.0", + "d3-array": "^1", + "d3-format": "^1", + "d3-time-format": "^2", "node-fetch": "3.3.2", + "safe-buffer": "5.2.1", + "tmp": "0.2.3", "undici": "6.21.2" }, "devDependencies": { @@ -86,12 +91,7 @@ "@types/d3-time-format": "2.3.4", "@types/node": "^18", "@types/xmldom": "0.1.34", - "d3-array": "^1", - "d3-format": "^1", - "d3-time-format": "^2", "data-uri-to-buffer": "6.0.2", - "safe-buffer": "5.2.1", - "tmp": "0.2.3", "soap": "1.1.10", "typescript-formatter": "^7.2.2" }, diff --git a/packages/comms/src/clienttools/eclMeta.ts b/packages/comms/src/clienttools/eclMeta.ts index 6d85d28751..582d663fbd 100644 --- a/packages/comms/src/clienttools/eclMeta.ts +++ b/packages/comms/src/clienttools/eclMeta.ts @@ -1,5 +1,5 @@ -import * as fs from "fs"; -import * as path from "path"; +import * as fs from "node:fs"; +import * as path from "node:path"; import { Dictionary, DictionaryNoCase, find, SAXStackParser, scopedLogger, XMLNode } from "@hpcc-js/util"; import { ClientTools, locateClientTools } from "./eclcc.ts"; diff --git a/packages/comms/src/clienttools/eclcc.ts b/packages/comms/src/clienttools/eclcc.ts index 12ea12497b..6d2192a9a8 100644 --- a/packages/comms/src/clienttools/eclcc.ts +++ b/packages/comms/src/clienttools/eclcc.ts @@ -1,7 +1,7 @@ -import * as cp from "child_process"; -import * as fs from "fs"; -import * as os from "os"; -import * as path from "path"; +import * as cp from "node:child_process"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; import * as tmp from "tmp"; import { exists, scopedLogger, xml2json, XMLNode } from "@hpcc-js/util"; diff --git a/packages/comms/src/index.node.ts b/packages/comms/src/index.node.ts index 24a1beb204..391fecbd8f 100644 --- a/packages/comms/src/index.node.ts +++ b/packages/comms/src/index.node.ts @@ -6,7 +6,7 @@ root.DOMParser = DOMParser; // fetch polyfill --- import fetch, { Headers, Request, Response, } from "node-fetch"; -import * as https from "https"; +import * as https from "node:https"; import { Agent, setGlobalDispatcher } from "undici"; if (typeof root.fetch === "undefined") { diff --git a/packages/markdown-it-plugins/package.json b/packages/markdown-it-plugins/package.json index 3d0b281815..8652aad4e4 100644 --- a/packages/markdown-it-plugins/package.json +++ b/packages/markdown-it-plugins/package.json @@ -58,15 +58,15 @@ "@types/markdown-it": "14.1.2" }, "peerDependencies": { - "markdown-it": "14.1.0" + "apache-arrow": "19.0.1", + "markdown-it": "14.1.0", + "shiki": "2.5.0" }, "devDependencies": { "@hpcc-js/esbuild-plugins": "^1.4.2", - "apache-arrow": "19.0.1", "d3-dsv": "3.0.1", "d3-fetch": "3.0.1", "dotenv": "16.4.7", - "shiki": "2.5.0", "tsx": "4.19.3" }, "repository": { diff --git a/tests/type-leaks/type-dependencies.spec.ts b/tests/type-leaks/type-dependencies.spec.ts index 493aac93f6..c40f926939 100644 --- a/tests/type-leaks/type-dependencies.spec.ts +++ b/tests/type-leaks/type-dependencies.spec.ts @@ -1,10 +1,13 @@ import { describe, it, expect } from "vitest"; import { existsSync, readFileSync, readdirSync, statSync } from "fs"; -import { join, resolve } from "path"; +import { join, resolve, dirname } from "path"; +import { fileURLToPath } from "url"; import { glob } from "glob"; import { parse } from "@typescript-eslint/typescript-estree"; -const PACKAGES_DIR = resolve("../../packages"); +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); +const PACKAGES_DIR = resolve(__dirname, "../../packages"); interface PackageInfo { name: string; @@ -88,6 +91,19 @@ function extractTypeReferences(filePath: string): Set { } } + // Export declarations (for re-exports like `export * from "package"`) + if (node.type === "ExportAllDeclaration" || node.type === "ExportNamedDeclaration") { + if (node.source && node.source.value) { + const exportSource = node.source.value; + if (!exportSource.startsWith(".") && !exportSource.startsWith("/")) { + const packageName = extractPackageName(exportSource); + if (packageName) { + typeReferences.add(packageName); + } + } + } + } + // Dynamic imports if (node.type === "ImportExpression" && node.source?.value) { const importSource = node.source.value; @@ -117,20 +133,26 @@ function extractTypeReferences(filePath: string): Set { // Ignore parse errors for now - we'll still catch obvious import statements console.warn(`Failed to parse ${filePath}:`, error); - // Fallback: use regex to find import statements + // Fallback: use regex to find import and export statements const content = readFileSync(filePath, "utf8"); - const importRegex = /(?:import|from|require\()\s*["\']([^"\']+)["\']/g; - let match; - - while ((match = importRegex.exec(content)) !== null) { - const importSource = match[1]; - if (!importSource.startsWith(".") && !importSource.startsWith("/")) { - const packageName = extractPackageName(importSource); - if (packageName) { - typeReferences.add(packageName); + const patterns = [ + IMPORT_EXPORT_PATTERNS.general, + IMPORT_EXPORT_PATTERNS.exportAll, + IMPORT_EXPORT_PATTERNS.exportNamed + ]; + + patterns.forEach(pattern => { + let match; + while ((match = pattern.exec(content)) !== null) { + const importSource = match[1]; + if (!importSource.startsWith(".") && !importSource.startsWith("/")) { + const packageName = extractPackageName(importSource); + if (packageName) { + typeReferences.add(packageName); + } } } - } + }); } return typeReferences; @@ -154,95 +176,232 @@ function extractPackageName(importPath: string): string | null { return null; } +/** + * Common regex patterns for extracting import/export statements + */ +const IMPORT_EXPORT_PATTERNS = { + // ES6 imports: import ... from "package" (not template literals) + esImport: /\bimport\s+[^'"]*\s+from\s+["']([^"'${}]+)["']/g, + // CommonJS require: require("package") (not template literals, not property access) + commonjsRequire: /(? { + const runtimeReferences = new Set(); + + try { + const content = readFileSync(filePath, "utf8"); + + // More precise regex patterns to match import/require statements in bundled files + const patterns = [ + IMPORT_EXPORT_PATTERNS.esImport, + IMPORT_EXPORT_PATTERNS.commonjsRequire, + IMPORT_EXPORT_PATTERNS.dynamicImport + ]; + + // AMD/UMD define dependencies need special handling + const defineRegex = IMPORT_EXPORT_PATTERNS.amdDefine; + let defineMatch; + while ((defineMatch = defineRegex.exec(content)) !== null) { + const depList = defineMatch[1]; + // Split by comma but be careful about quoted strings + const deps = depList.match(/["'][^"']*["']/g) || []; + for (const quotedDep of deps) { + const dep = quotedDep.slice(1, -1); // Remove quotes + if (dep && !dep.startsWith(".") && !dep.startsWith("/") && + !dep.includes("${") && !dep.includes("'") && !dep.includes('"') && + !dep.includes(" ") && dep.length < 50 && // Avoid false positives from long code snippets + /^[a-zA-Z0-9@/_-]+$/.test(dep)) { // Only valid package name characters + const packageName = extractPackageName(dep); + if (packageName && packageName !== "exports" && packageName !== "module" && + !["value", "dojo.parser"].includes(packageName)) { + runtimeReferences.add(packageName); + } + } + } + } + + // Process other patterns + for (const pattern of patterns) { + let match; + while ((match = pattern.exec(content)) !== null) { + const importSource = match[1]; + if (importSource && !importSource.startsWith(".") && !importSource.startsWith("/") && + !importSource.includes("${") && importSource.length < 100) { + const packageName = extractPackageName(importSource); + if (packageName && packageName !== "exports" && packageName !== "module") { + runtimeReferences.add(packageName); + } + } + } + } + } catch (error) { + console.warn(`Failed to read bundled file ${filePath}:`, error); + } + + return runtimeReferences; +} + +describe("Package Dependencies", () => { + const packages = getPackages(); + + it("should have valid package.json files", () => { + expect(packages.length).toBeGreaterThan(0); + packages.forEach(pkg => { + expect(pkg.name).toBeTruthy(); + expect(pkg.packageJson).toBeTruthy(); + }); + }); + + it("should not have type dependencies that are not declared as dependencies or devDependencies", () => { + const errors: string[] = []; + + packages.forEach(pkg => { + const srcDir = join(pkg.path, "src"); + if (!existsSync(srcDir)) return; + + const tsFiles = glob.sync("**/*.ts", { + cwd: srcDir, + absolute: true, + ignore: ["**/*.d.ts", "**/*.spec.ts", "**/*.test.ts"] + }); + + const allTypeReferences = new Set(); + tsFiles.forEach(file => { + const refs = extractTypeReferences(file); + refs.forEach(ref => allTypeReferences.add(ref)); + }); + + allTypeReferences.forEach(ref => { + if (!pkg.dependencies.has(ref) && + !pkg.devDependencies.has(ref) && + !pkg.peerDependencies.has(ref)) { + errors.push(`${pkg.name}: Type reference '${ref}' not found in dependencies, devDependencies, or peerDependencies`); + } + }); + }); + + if (errors.length > 0) { + throw new Error(`Type dependency violations found:\n${errors.join("\n")}`); + } + }); + + it("should not reference third-party runtime dependencies unless they are in dependencies", () => { + const errors: string[] = []; + + // Known build-time/dev-only packages that should be ignored + const knownBuildTimeDeps = new Set(["dojo", "dgrid", "dojo.parser", "function", "some"]); + + packages.forEach(pkg => { + // Check for bundled files in common output directories + const bundleDirs = ["dist"]; + const bundledFiles: string[] = []; + + bundleDirs.forEach(dir => { + const bundleDir = join(pkg.path, dir); + if (existsSync(bundleDir)) { + const jsFiles = glob.sync("**/*.{js,mjs,cjs}", { + cwd: bundleDir, + absolute: true, + ignore: ["**/*.min.js", "**/*.map", "**/*.d.ts"] + }); + bundledFiles.push(...jsFiles); + } + }); + + if (bundledFiles.length === 0) return; + + const allRuntimeReferences = new Set(); + bundledFiles.forEach(file => { + const refs = extractRuntimeReferences(file); + refs.forEach(ref => allRuntimeReferences.add(ref)); + }); + + // Filter out built-in modules, internal @hpcc-js packages, and known build-time deps + const externalReferences = Array.from(allRuntimeReferences).filter(ref => + !isBuiltInModule(ref) && + !knownBuildTimeDeps.has(ref) + ); + + externalReferences.forEach(ref => { + if (!pkg.dependencies.has(ref) && !pkg.peerDependencies.has(ref)) { + errors.push(`${pkg.name}: Runtime reference '${ref}' not found in dependencies or peerDependencies (found in bundled files)`); + } + }); + }); + + if (errors.length > 0) { + throw new Error(`Runtime dependency violations found:\n${errors.join("\n")}`); + } + }); +}); + describe("Package Type Dependencies", () => { const packages = getPackages(); for (const pkg of packages) { describe(`${pkg.name}`, () => { it("should not reference third-party types unless they are in dependencies", async () => { - // Get all TypeScript files in the package - const tsFiles = ["types/index.d.ts", "types/index.browser.d.ts", "types/index.node.d.ts"]; + // Get all TypeScript declaration files in the package + const typesDir = join(pkg.path, "types"); + if (!existsSync(typesDir)) return; + + const tsFiles = glob.sync("**/index*.d.ts", { + cwd: typesDir, + absolute: true, + ignore: ["**/*.spec.d.ts", "**/*.test.d.ts"] + }); const allTypeReferences = new Set(); - // Extract type references from all TypeScript files + // Extract type references from all TypeScript declaration files for (const tsFile of tsFiles) { - const filePath = join(pkg.path, tsFile); - if (existsSync(filePath)) { - const typeRefs = extractTypeReferences(filePath); - typeRefs.forEach(ref => allTypeReferences.add(ref)); - } + const typeRefs = extractTypeReferences(tsFile); + typeRefs.forEach(ref => allTypeReferences.add(ref)); } // Check each type reference const violations: string[] = []; for (const typeRef of allTypeReferences) { - // Skip if it's a built-in module - if (isBuiltInModule(typeRef)) { - continue; - } - - // Skip if it's an internal @hpcc-js package - if (typeRef.startsWith("@hpcc-js/")) { - continue; - } - // Skip if it's in dependencies if (pkg.dependencies.has(typeRef)) { continue; } - // Skip if it's in dependencies + // Skip if it's in peerDependencies if (pkg.peerDependencies.has(typeRef)) { continue; } - // Skip if it's a @types/ package and the base package is in dependencies - if (typeRef.startsWith("@types/")) { - const basePackage = typeRef.replace("@types/", ""); - if (pkg.dependencies.has(basePackage)) { + // Skip if it's a regular package but we have its @types/ equivalent in dependencies + if (!typeRef.startsWith("@types/")) { + const typesPackage = `@types/${typeRef}`; + if (pkg.dependencies.has(typesPackage) || pkg.peerDependencies.has(typesPackage)) { continue; } } - // Skip vitest-related packages (test runner) - if (typeRef === "vitest" || typeRef.startsWith("@vitest/")) { - continue; - } - - // Skip some common dev-only packages that might be referenced in source - const devOnlyPackages = new Set([ - "vitepress", "vite", "typescript", "esbuild", - "@vitejs/plugin-react", "rollup" - ]); - - if (devOnlyPackages.has(typeRef)) { - continue; - } - // This is a violation - third-party type used without dependency violations.push(typeRef); } @@ -250,6 +409,7 @@ describe("Package Type Dependencies", () => { if (violations.length > 0) { console.error(`\n${pkg.name} type violations:`); console.error(`Dependencies: ${Array.from(pkg.dependencies).join(", ") || "(none)"}`); + console.error(`PeerDependencies: ${Array.from(pkg.peerDependencies).join(", ") || "(none)"}`); console.error(`Violations: ${violations.join(", ")}`); } @@ -258,3 +418,4 @@ describe("Package Type Dependencies", () => { }); } }); +