From 689ccfcb17b007b270028197d01451d0352d44ae Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Thu, 30 Mar 2023 15:51:03 -0700 Subject: [PATCH] WIP dtslint checks I'm at the point of checking `version` and I think I need to copy code from definition-parser maybe. Visions of semver parsing... --- packages/dtslint/src/checks.ts | 85 +++++++++++-------- packages/dtslint/src/index.ts | 17 ++-- packages/dtslint/src/lint.ts | 18 ++-- packages/dtslint/src/util.ts | 16 ++-- packages/dtslint/test/index.test.ts | 125 +++++++++++++++++++++------- 5 files changed, 173 insertions(+), 88 deletions(-) diff --git a/packages/dtslint/src/checks.ts b/packages/dtslint/src/checks.ts index e7d076fe..a2d6fbe8 100644 --- a/packages/dtslint/src/checks.ts +++ b/packages/dtslint/src/checks.ts @@ -1,38 +1,48 @@ import { makeTypesVersionsForPackageJson } from "@definitelytyped/header-parser"; import { TypeScriptVersion } from "@definitelytyped/typescript-versions"; import assert = require("assert"); -import { pathExists } from "fs-extra"; +import { pathExistsSync } from "fs-extra"; import { join as joinPaths } from "path"; import { CompilerOptions } from "typescript"; -import { readJson } from "./util"; - -export async function checkPackageJson(dirPath: string, typesVersions: readonly TypeScriptVersion[]): Promise { +import { readJson, packageNameFromPath } from "./util"; +export function checkPackageJson(dirPath: string, typesVersions: readonly TypeScriptVersion[]): string[] { const pkgJsonPath = joinPaths(dirPath, "package.json"); - const needsTypesVersions = typesVersions.length !== 0; - if (!(await pathExists(pkgJsonPath))) { - if (needsTypesVersions) { - throw new Error(`${dirPath}: Must have 'package.json' for "typesVersions"`); - } - return; + if (!pathExistsSync(pkgJsonPath)) { + throw new Error(`${dirPath}: Missing 'package.json'`); } + return checkPackageJsonContents(dirPath, readJson(pkgJsonPath), typesVersions); +} - const pkgJson = (await readJson(pkgJsonPath)) as Record; - - if ((pkgJson as any).private !== true) { - throw new Error(`${pkgJsonPath} should set \`"private": true\``); +export function checkPackageJsonContents(dirPath: string, pkgJson: Record, typesVersions: readonly TypeScriptVersion[]): string[] { + const errors = [] + const pkgJsonPath = joinPaths(dirPath, "package.json"); + const packageName = packageNameFromPath(dirPath); + const needsTypesVersions = typesVersions.length !== 0; + if (pkgJson.private !== true) { + errors.push(`${pkgJsonPath} should have \`"private": true\``) + } + if (pkgJson.name !== "@types/" + packageName) { + errors.push(`${pkgJsonPath} should have \`"name": "@types/${packageName}"\``); + } + if (typeof pkgJson.devDependencies !== "object" + || pkgJson.devDependencies === null + || (pkgJson.devDependencies as any)["@types/" + packageName] !== "link:.") { + errors.push(`In ${pkgJsonPath}, devDependencies must include \`"@types/${packageName}": "link:."\``); } if (needsTypesVersions) { - assert.strictEqual((pkgJson as any).types, "index", `"types" in '${pkgJsonPath}' should be "index".`); + assert.strictEqual(pkgJson.types, "index", `"types" in '${pkgJsonPath}' should be "index".`); const expected = makeTypesVersionsForPackageJson(typesVersions) as Record; - assert.deepEqual( - (pkgJson as any).typesVersions, - expected, - `"typesVersions" in '${pkgJsonPath}' is not set right. Should be: ${JSON.stringify(expected, undefined, 4)}` - ); + if (!deepEquals(pkgJson.typesVersions, expected)) { + errors.push(`"typesVersions" in '${pkgJsonPath}' is not set right. Should be: ${JSON.stringify(expected, undefined, 4)}`) + } } - + // TODO: This needs to be much longer + // TODO: Add checks for name and the other new field(s) + // TODO: Should they be disablable, in lint rules? I mean, if we have the data right now, the answer is NO + // TODO: If these aren't going to be lint rules, they should collect errors instead of throwing. Fixing one thing at a time sucks. + // TODO: Test on a toplevel package, a scoped package, and old-version package, and an old-TS-version package for (const key in pkgJson) { // tslint:disable-line forin switch (key) { @@ -44,19 +54,21 @@ export async function checkPackageJson(dirPath: string, typesVersions: readonly case "type": case "name": case "version": + case "devDependencies": // "private"/"typesVersions"/"types" checked above, "dependencies" / "license" checked by types-publisher, - // TODO: "name"/"version" checked by types-publisher/CI. + // TODO: "name"/"version" checked above, plus asserts in types-publisher break; case "typesVersions": case "types": if (!needsTypesVersions) { - throw new Error(`${pkgJsonPath} doesn't need to set "${key}" when no 'ts3.x' directories exist.`); + errors.push(`${pkgJsonPath} doesn't need to set "${key}" when no 'ts3.x' directories exist.`); } break; default: - throw new Error(`${pkgJsonPath} should not include field ${key}`); + errors.push(`${pkgJsonPath} should not include field ${key}`); } } + return errors } /** @@ -72,7 +84,9 @@ export interface DefinitelyTypedInfo { /** "../" or "../../" or "../../../". This should use '/' even on windows. */ readonly relativeBaseUrl: string; } -export function checkTsconfig(options: CompilerOptionsRaw, dt: boolean): void { +// TODO: Maybe check ALL of tsconfig, not just compilerOptions +export function checkTsconfig(options: CompilerOptionsRaw, dt: boolean): string[] { + const errors = [] if (dt) { const mustHave = { noEmit: true, @@ -84,7 +98,7 @@ export function checkTsconfig(options: CompilerOptionsRaw, dt: boolean): void { const expected = mustHave[key]; const actual = options[key]; if (!deepEquals(expected, actual)) { - throw new Error( + errors.push( `Expected compilerOptions[${JSON.stringify(key)}] === ${JSON.stringify(expected)}, but got ${JSON.stringify( actual )}` @@ -103,7 +117,6 @@ export function checkTsconfig(options: CompilerOptionsRaw, dt: boolean): void { case "strictFunctionTypes": case "esModuleInterop": case "allowSyntheticDefaultImports": - case "paths": case "target": case "jsx": case "jsxFactory": @@ -115,29 +128,28 @@ export function checkTsconfig(options: CompilerOptionsRaw, dt: boolean): void { break; default: if (!(key in mustHave)) { - throw new Error(`Unexpected compiler option ${key}`); + errors.push(`Unexpected compiler option ${key}`); } } } } - if (!("lib" in options)) { - throw new Error('Must specify "lib", usually to `"lib": ["es6"]` or `"lib": ["es6", "dom"]`.'); + errors.push('Must specify "lib", usually to `"lib": ["es6"]` or `"lib": ["es6", "dom"]`.'); } if (!("module" in options)) { - throw new Error('Must specify "module" to `"module": "commonjs"` or `"module": "node16"`.'); + errors.push('Must specify "module" to `"module": "commonjs"` or `"module": "node16"`.'); } if ( options.module?.toString().toLowerCase() !== "commonjs" && options.module?.toString().toLowerCase() !== "node16" ) { - throw new Error(`When "module" is present, it must be set to "commonjs" or "node16".`); + errors.push(`When "module" is present, it must be set to "commonjs" or "node16".`); } if ("strict" in options) { if (options.strict !== true) { - throw new Error('When "strict" is present, it must be set to `true`.'); + errors.push('When "strict" is present, it must be set to `true`.'); } for (const key of ["noImplicitAny", "noImplicitThis", "strictNullChecks", "strictFunctionTypes"]) { @@ -148,22 +160,23 @@ export function checkTsconfig(options: CompilerOptionsRaw, dt: boolean): void { } else { for (const key of ["noImplicitAny", "noImplicitThis", "strictNullChecks", "strictFunctionTypes"]) { if (!(key in options)) { - throw new Error(`Expected \`"${key}": true\` or \`"${key}": false\`.`); + errors.push(`Expected \`"${key}": true\` or \`"${key}": false\`.`); } } } if ("exactOptionalPropertyTypes" in options) { if (options.exactOptionalPropertyTypes !== true) { - throw new Error('When "exactOptionalPropertyTypes" is present, it must be set to `true`.'); + errors.push('When "exactOptionalPropertyTypes" is present, it must be set to `true`.'); } } if (options.types && options.types.length) { - throw new Error( + errors.push( 'Use `/// ` directives in source files and ensure ' + 'that the "types" field in your tsconfig is an empty array.' ); } + return errors; } function deepEquals(expected: unknown, actual: unknown): boolean { diff --git a/packages/dtslint/src/index.ts b/packages/dtslint/src/index.ts index b5927128..70967f11 100644 --- a/packages/dtslint/src/index.ts +++ b/packages/dtslint/src/index.ts @@ -9,7 +9,7 @@ import { basename, dirname, join as joinPaths, resolve } from "path"; import { cleanTypeScriptInstalls, installAllTypeScriptVersions, installTypeScriptNext } from "@definitelytyped/utils"; import { checkPackageJson, checkTsconfig } from "./checks"; import { checkTslintJson, lint, TsVersion } from "./lint"; -import { getCompilerOptions, mapDefinedAsync, withoutPrefix } from "./util"; +import { getCompilerOptions, mapDefinedAsync, packageNameFromPath, withoutPrefix } from "./util"; async function main(): Promise { const args = process.argv.slice(2); @@ -132,12 +132,13 @@ async function runTests( ): Promise { const indexText = await readFile(joinPaths(dirPath, "index.d.ts"), "utf-8"); // If this *is* on DefinitelyTyped, types-publisher will fail if it can't parse the header. + // TODO: We really shouldn't be running off DT anymore. const dt = indexText.includes("// Type definitions for"); if (dt) { // Someone may have copied text from DefinitelyTyped to their type definition and included a header, // so assert that we're really on DefinitelyTyped. const dtRoot = findDTRoot(dirPath); - const packageName = basename(dirPath); + const packageName = packageNameFromPath(dirPath); assertPathIsInDefinitelyTyped(dirPath, dtRoot); assertPathIsNotBanned(packageName); assertPackageIsNotDeprecated(packageName, await readFile(joinPaths(dtRoot, "notNeededPackages.json"), "utf-8")); @@ -162,7 +163,10 @@ async function runTests( }); if (dt) { - await checkPackageJson(dirPath, typesVersions); + const packageJsonErrors = checkPackageJson(dirPath, typesVersions); + if (packageJsonErrors.length > 0) { + throw new Error(packageJsonErrors.join("\n")) + } } const minVersion = maxVersion( @@ -222,8 +226,11 @@ async function testTypesVersion( tsLocal: string | undefined, isLatest: boolean ): Promise { - await checkTslintJson(dirPath, dt); - checkTsconfig(await getCompilerOptions(dirPath), dt); + checkTslintJson(dirPath, dt); + const tsconfigErrors = checkTsconfig(getCompilerOptions(dirPath), dt); + if (tsconfigErrors.length > 0) { + throw new Error(tsconfigErrors.join("\n")) + } const err = await lint(dirPath, lowVersion, hiVersion, isLatest, expectOnly, tsLocal); if (err) { throw new Error(err); diff --git a/packages/dtslint/src/lint.ts b/packages/dtslint/src/lint.ts index ece7c92c..45bbf4c8 100644 --- a/packages/dtslint/src/lint.ts +++ b/packages/dtslint/src/lint.ts @@ -1,7 +1,7 @@ import { TypeScriptVersion } from "@definitelytyped/typescript-versions"; import { typeScriptPath } from "@definitelytyped/utils"; import assert = require("assert"); -import { pathExists } from "fs-extra"; +import { pathExistsSync } from "fs-extra"; import { dirname, join as joinPaths, normalize } from "path"; import { Configuration, Linter } from "tslint"; import { ESLint } from "eslint"; @@ -37,7 +37,7 @@ export async function lint( const configPath = expectOnly ? joinPaths(__dirname, "..", "dtslint-expect-only.json") : getConfigPath(dirPath); // TODO: To port expect-rule, eslint's config will also need to include [minVersion, maxVersion] // Also: expect-rule should be renamed to expect-type or check-type or something - const config = await getLintConfig(configPath, tsconfigPath, minVersion, maxVersion, tsLocal); + const config = getLintConfig(configPath, tsconfigPath, minVersion, maxVersion, tsLocal); const esfiles = []; for (const file of lintProgram.getSourceFiles()) { @@ -190,13 +190,13 @@ function testNoLintDisables(disabler: "tslint:disable" | "eslint-disable", text: } } -export async function checkTslintJson(dirPath: string, dt: boolean): Promise { +export function checkTslintJson(dirPath: string, dt: boolean): void { const configPath = getConfigPath(dirPath); const shouldExtend = `@definitelytyped/dtslint/${dt ? "dt" : "dtslint"}.json`; const validateExtends = (extend: string | string[]) => extend === shouldExtend || (!dt && Array.isArray(extend) && extend.some((val) => val === shouldExtend)); - if (!(await pathExists(configPath))) { + if (!pathExistsSync(configPath)) { if (dt) { throw new Error( `On DefinitelyTyped, must include \`tslint.json\` containing \`{ "extends": "${shouldExtend}" }\`.\n` + @@ -206,8 +206,8 @@ export async function checkTslintJson(dirPath: string, dt: boolean): Promise { - const configExists = await pathExists(expectedConfigPath); +): IConfigurationFile { + const configExists = pathExistsSync(expectedConfigPath); const configPath = configExists ? expectedConfigPath : joinPaths(__dirname, "..", "dtslint.json"); // Second param to `findConfiguration` doesn't matter, since config path is provided. const config = Configuration.findConfiguration(configPath, "").results; diff --git a/packages/dtslint/src/util.ts b/packages/dtslint/src/util.ts index 4ddfe6c3..85e6c50c 100644 --- a/packages/dtslint/src/util.ts +++ b/packages/dtslint/src/util.ts @@ -1,6 +1,6 @@ import { ESLintUtils } from "@typescript-eslint/utils"; import assert = require("assert"); -import { pathExists, readFile } from "fs-extra"; +import { pathExistsSync, readFileSync } from "fs-extra"; import { basename, dirname, join } from "path"; import stripJsonComments = require("strip-json-comments"); import * as ts from "typescript"; @@ -9,9 +9,11 @@ export const createRule = ESLintUtils.RuleCreator( (name) => `https://github.com/microsoft/DefinitelyTyped-tools/tree/master/packages/dtslint/src/rules/${name}.ts` ); -export async function readJson(path: string) { - const text = await readFile(path, "utf-8"); - return JSON.parse(stripJsonComments(text)); +export function packageNameFromPath(path: string): string { + return /^v\d+(\.\d+)?$/.exec(path) || /^ts\d\.\d/.exec(path) ? basename(dirname(path)) : basename(path) +} +export function readJson(path: string): Record { + return JSON.parse(stripJsonComments(readFileSync(path, "utf-8"))); } export function failure(ruleName: string, s: string): string { @@ -31,12 +33,12 @@ export function getCommonDirectoryName(files: readonly string[]): string { return basename(minDir); } -export async function getCompilerOptions(dirPath: string): Promise { +export function getCompilerOptions(dirPath: string): ts.CompilerOptions { const tsconfigPath = join(dirPath, "tsconfig.json"); - if (!(await pathExists(tsconfigPath))) { + if (!pathExistsSync(tsconfigPath)) { throw new Error(`Need a 'tsconfig.json' file in ${dirPath}`); } - return (await readJson(tsconfigPath)).compilerOptions; + return readJson(tsconfigPath).compilerOptions as ts.CompilerOptions; } export function withoutPrefix(s: string, prefix: string): string | undefined { diff --git a/packages/dtslint/test/index.test.ts b/packages/dtslint/test/index.test.ts index ff855870..22047b0d 100644 --- a/packages/dtslint/test/index.test.ts +++ b/packages/dtslint/test/index.test.ts @@ -2,7 +2,7 @@ import { join } from "path"; import { consoleTestResultHandler, runTest } from "tslint/lib/test"; import { existsSync, readdirSync, statSync } from "fs"; -import { CompilerOptionsRaw, checkTsconfig } from "../src/checks"; +import { CompilerOptionsRaw, checkPackageJsonContents, checkTsconfig } from "../src/checks"; import { assertPackageIsNotDeprecated } from "../src/index"; const testDir = __dirname; @@ -37,43 +37,106 @@ describe("dtslint", () => { noImplicitThis: true, strictNullChecks: true, strictFunctionTypes: true, - baseUrl: "../", - typeRoots: ["../"], types: [], noEmit: true, forceConsistentCasingInFileNames: true, }; + const pkgJson: Record = { + "private": true, + "name": "@types/hapi", + "version": "18.0.0", + "dependencies": { + "@types/boom": "*", + "@types/catbox": "*", + "@types/iron": "*", + "@types/mimos": "*", + "@types/node": "*", + "@types/podium": "*", + "@types/shot": "*", + "joi": "^17.3.0" + }, + "devDependencies": { + "@types/hapi": "link:." + } + } describe("checks", () => { - it("disallows unknown compiler options", () => { - expect(() => checkTsconfig({ ...base, completelyInvented: true }, { relativeBaseUrl: "../" })).toThrow( - "Unexpected compiler option completelyInvented" - ); + describe("checkTsconfig", () => { + it("disallows unknown compiler options", () => { + expect(checkTsconfig({ ...base, completelyInvented: true }, true)).toEqual([ + "Unexpected compiler option completelyInvented" + ]); + }); + it("allows exactOptionalPropertyTypes: true", () => { + expect(checkTsconfig({ ...base, exactOptionalPropertyTypes: true }, true)).toEqual([]); + }); + it("allows module: node16", () => { + expect(checkTsconfig({ ...base, module: "node16" }, true)).toEqual([]); + }); + it("disallows missing `module`", () => { + const options = { ...base }; + delete options.module; + expect(checkTsconfig(options, true)).toEqual([ + 'Must specify "module" to `"module": "commonjs"` or `"module": "node16"`.' + ]); + }); + it("disallows exactOptionalPropertyTypes: false", () => { + expect(checkTsconfig({ ...base, exactOptionalPropertyTypes: false }, true)).toEqual([ + 'When "exactOptionalPropertyTypes" is present, it must be set to `true`.' + ]); + }); + it("disallows `paths`", () => { + expect(checkTsconfig({ ...base, paths: { "c": ['.'] } }, true)).toEqual([ + 'Unexpected compiler option paths' + ]); + }); }); - it("allows exactOptionalPropertyTypes: true", () => { - expect(checkTsconfig({ ...base, exactOptionalPropertyTypes: true }, { relativeBaseUrl: "../" })).toBeFalsy(); + describe("checkPackageJson", () => { + it("requires private: true", () => { + const pkg = { ...pkgJson }; + delete pkg.private; + expect(checkPackageJsonContents("cort-start/hapi", pkg, [])).toEqual([ + "cort-start/hapi/package.json should have `\"private\": true`" + ]); + }); + it("requires name", () => { + const pkg = { ...pkgJson }; + delete pkg.name; + expect(checkPackageJsonContents("cort-start/basis", pkg, [])).toEqual([ + "cort-start/basis/package.json should have `\"name\": \"@types/basis\"`" + ]); + }); + it("requires name to match", () => { + expect(checkPackageJsonContents("cort-start/basis", pkgJson, [])).toEqual([ + "cort-start/basis/package.json should have `\"name\": \"@types/basis\"`" + ]); + }); + it("requires devDependencies", () => { + const pkg = { ...pkgJson }; + delete pkg.devDependencies; + expect(checkPackageJsonContents("cort-start/hapi", pkg, [])).toEqual([ + `In cort-start/hapi/package.json, devDependencies must include \`"@types/hapi": "link:."\`` + ]); + }); + it("requires devDependencies to contain self-package", () => { + expect(checkPackageJsonContents("cort-start/hapi", { ...pkgJson, devDependencies: { } }, [])).toEqual([ + `In cort-start/hapi/package.json, devDependencies must include \`"@types/hapi": "link:."\`` + ]); + }); + it("requires devDependencies to contain self-package version 'link:.'", () => { + expect(checkPackageJsonContents("cort-start/hapi", { ...pkgJson, devDependencies: { "@types/hapi": "*" } }, [])).toEqual([ + `In cort-start/hapi/package.json, devDependencies must include \`"@types/hapi": "link:."\`` + ]); + }); }); - it("allows module: node16", () => { - expect(checkTsconfig({ ...base, module: "node16" }, { relativeBaseUrl: "../" })).toBeFalsy(); - }); - it("disallows missing `module`", () => { - const options = { ...base }; - delete options.module; - expect(() => checkTsconfig(options, { relativeBaseUrl: "../" })).toThrow( - 'Must specify "module" to `"module": "commonjs"` or `"module": "node16"`.' - ); - }); - it("disallows exactOptionalPropertyTypes: false", () => { - expect(() => checkTsconfig({ ...base, exactOptionalPropertyTypes: false }, { relativeBaseUrl: "../" })).toThrow( - 'When "exactOptionalPropertyTypes" is present, it must be set to `true`.' - ); - }); - it("disallows packages that are in notNeededPackages.json", () => { - expect(() => assertPackageIsNotDeprecated("foo", '{ "packages": { "foo": { } } }')).toThrow( - "notNeededPackages.json has an entry for foo." - ); - }); - it("allows packages that are not in notNeededPackages.json", () => { - expect(assertPackageIsNotDeprecated("foo", '{ "packages": { "bar": { } } }')).toBeUndefined(); + describe("assertPackageIsNotDeprecated", () => { + it("disallows packages that are in notNeededPackages.json", () => { + expect(() => assertPackageIsNotDeprecated("foo", '{ "packages": { "foo": { } } }')).toThrow( + "notNeededPackages.json has an entry for foo." + ); + }); + it("allows packages that are not in notNeededPackages.json", () => { + expect(assertPackageIsNotDeprecated("foo", '{ "packages": { "bar": { } } }')).toBeUndefined(); + }); }); }); describe("rules", () => {