feat: Remove pkg from expectedNpmVersionFailures by hacking dtslint

[hkleungai]

Aim to remove unneeded packages from expectedNpmVersionFailures.txt, by hacking current dtslint implementation to check whether or not target packages will emit "... can be removed from expectedNpmVersionFailures.txt in ..." warning.

Step 1. Add packages/dtslint/src/check-package-in-expectedVersionFailures.ts into DT-tools repo, and run pnpm build. Note: Core logics are copied from packages/dtslint/src/index.ts. Hope they are correcly done :)

#!/usr/bin/env node

import { getTypesVersions } from "@definitelytyped/header-parser";
import fs from "fs";
import { basename, dirname, join as joinPaths } from "path";
import {
  checkNpmVersionAndGetMatchingImplementationPackage,
  checkPackageJson,
} from "./checks";
import { packageDirectoryNameWithVersionFromPath, packageNameFromPath } from "./util";

async function main(): Promise<
  | { isRemovableFromExpectedNpmVersionFailures: boolean } 
  | void
> {
  const args = process.argv.slice(2);
  let dirPath = process.cwd();

  console.log(`dtslint@${require("../package.json").version}`);
  if (args.length === 1 && args[0] === "types") {
    console.log(
      "Please provide a package name to test.\nTo test all changed packages at once, run `pnpm run test-all`.",
    );
    process.exit(1);
  }

  for (const arg of args) {
    switch (arg) {
      default: {
        if (arg.startsWith("--")) {
          console.error(`Unknown option '${arg}'`);
          process.exit(1);
        }

        const path =
          arg.indexOf("@") === 0 && arg.indexOf("/") !== -1
            ? // we have a scoped module, e.g. @bla/foo
              // which should be converted to   bla__foo
              arg.slice(1).replace("/", "__")
            : arg;
        dirPath = joinPaths(dirPath, path);
      }
    }
  }

  const { 
    isRemovableFromExpectedNpmVersionFailures,
  } = await checkPackageInExpectedVersionFailures(dirPath);
  
  console.log(
    `${dirPath} isRemovableFromExpectedNpmVersionFailures? ${isRemovableFromExpectedNpmVersionFailures}`
  );
}

/**
 * @returns Warning text - should be displayed during the run, but does not indicate failure.
 */
async function checkPackageInExpectedVersionFailures(
  dirPath: string,
): Promise<{ isRemovableFromExpectedNpmVersionFailures: boolean }> {
  // Assert that we're really on DefinitelyTyped.
  const dtRoot = findDTRoot(dirPath);
  const packageName = packageNameFromPath(dirPath);
  const packageDirectoryNameWithVersion = packageDirectoryNameWithVersionFromPath(dirPath);
  assertPathIsInDefinitelyTyped(dirPath, dtRoot);
  assertPathIsNotBanned(packageName);
  assertPackageIsNotDeprecated(
    packageName,
    await fs.promises.readFile(joinPaths(dtRoot, "notNeededPackages.json"), "utf-8"),
  );

  const typesVersions = getTypesVersions(dirPath);
  const packageJson = checkPackageJson(dirPath, typesVersions);
  if (Array.isArray(packageJson)) {
    throw new Error("\n\t* " + packageJson.join("\n\t* "));
  }

  const warnings: string[] = [];

  const result = await checkNpmVersionAndGetMatchingImplementationPackage(
    packageJson,
    packageDirectoryNameWithVersion,
  );

  warnings.push(...result.warnings);

  return { 
    isRemovableFromExpectedNpmVersionFailures: warnings.some(warning => (
      warning.includes(" can be removed from expectedNpmVersionFailures.txt in ")
    )), 
  };
}

function findDTRoot(dirPath: string) {
  let path = dirPath;
  while (basename(path) !== "types" && dirname(path) !== "." && dirname(path) !== "/") {
    path = dirname(path);
  }
  return dirname(path);
}

function assertPathIsInDefinitelyTyped(dirPath: string, dtRoot: string): void {
  // TODO: It's not clear whether this assertion makes sense, and it's broken on Azure Pipelines (perhaps because DT isn't cloned into DefinitelyTyped)
  // Re-enable it later if it makes sense.
  // if (basename(dtRoot) !== "DefinitelyTyped")) {
  if (!fs.existsSync(joinPaths(dtRoot, "types"))) {
    throw new Error(
      "Since this type definition includes a header (a comment starting with `// Type definitions for`), " +
        "assumed this was a DefinitelyTyped package.\n" +
        "But it is not in a `DefinitelyTyped/types/xxx` directory: " +
        dirPath,
    );
  }
}

/**
 * Starting at some point in time, npm has banned all new packages whose names
 * contain the word `download`. However, some older packages exist that still
 * contain this name.
 * @NOTE for contributors: The list of literal exceptions below should ONLY be
 * extended with packages for which there already exists a corresponding type
 * definition package in the `@types` scope. More information:
 * https://github.com/microsoft/DefinitelyTyped-tools/pull/381.
 */
function assertPathIsNotBanned(packageName: string) {
  if (
    /(^|\W)download($|\W)/.test(packageName) &&
    packageName !== "download" &&
    packageName !== "downloadjs" &&
    packageName !== "s3-download-stream"
  ) {
    // Since npm won't release their banned-words list, we'll have to manually add to this list.
    throw new Error(`${packageName}: Contains the word 'download', which is banned by npm.`);
  }
}

export function assertPackageIsNotDeprecated(packageName: string, notNeededPackages: string) {
  const unneeded = JSON.parse(notNeededPackages).packages;
  if (Object.keys(unneeded).includes(packageName)) {
    throw new Error(`${packageName}: notNeededPackages.json has an entry for ${packageName}.
That means ${packageName} ships its own types, and @types/${packageName} was deprecated and removed from Definitely Typed.
If you want to re-add @types/${packageName}, please remove its entry from notNeededPackages.json.`);
  }
}

if (require.main === module) {
  main().catch((err) => {
    console.error(err.stack);
    process.exit(1);
  });
} else {
  module.exports = checkPackageInExpectedVersionFailures;
}

Step 2. Add update-expectedNpmVersionFailures-by-dtslint.js into DT repo, and run node ./update-expectedNpmVersionFailures-by-dtslint.js. Note: Async network calls firing in length-50 batches may (or may not) be too aggressive. Use it with cautious if you need to.

import fs from 'node:fs';
import { cwd } from 'node:process';
import { basename, dirname, join as joinPaths } from "node:path";

import checkPackageInExpectedVersionFailures from '../DefinitelyTyped-tools/packages/dtslint/dist/check-package-in-expectedVersionFailures.js';

const ATTW_PATH = './attw.json';
const EXPECTED_NPM_VERSION_FAILURES_PATH = '../DefinitelyTyped-tools/packages/dtslint/expectedNpmVersionFailures.txt';

const attw = fs.readFileSync(ATTW_PATH, { encoding: 'utf-8' });
const attwFailedPackageSet = new Set(JSON.parse(attw).failingPackages);

const currentExpectedNpmVersionFailures = (
    fs.readFileSync(
        EXPECTED_NPM_VERSION_FAILURES_PATH,
        { encoding: 'utf-8' },
    )
);
let updatedExpectedNpmVersionFailures;

{
    const currentDtToolsFailedPackageList = currentExpectedNpmVersionFailures.split('\n').filter(Boolean);
    const updatedDtToolsFailedPackageList = [];

    const REQ_BATCH_SIZE = 50;
    for (let i = 0; i < currentDtToolsFailedPackageList.length / REQ_BATCH_SIZE; i++) {
        const currentDtToolsFailedPackageBatch = currentDtToolsFailedPackageList.slice(i * REQ_BATCH_SIZE, (i + 1) * REQ_BATCH_SIZE);

        await Promise.all(currentDtToolsFailedPackageBatch.map(async currentDtToolsFailedPackage => {
            if (currentDtToolsFailedPackage === '') {
                updatedDtToolsFailedPackageList.push(currentDtToolsFailedPackage);
                return;
            }

            const {
                isRemovableFromExpectedNpmVersionFailures
            } = await checkPackageInExpectedVersionFailures(joinPaths(cwd(), 'types', currentDtToolsFailedPackage));
            if (!isRemovableFromExpectedNpmVersionFailures) {
                updatedDtToolsFailedPackageList.push(currentDtToolsFailedPackage);
            }
        }));
    }


    updatedExpectedNpmVersionFailures = (
        updatedDtToolsFailedPackageList
            .sort((a, b) => a.localeCompare(b))
            .join('\n')

    );
}

fs.writeFileSync(
    EXPECTED_NPM_VERSION_FAILURES_PATH,
    updatedExpectedNpmVersionFailures + '\n',
    { encoding: 'utf-8' }
);

Then packages emitting warning "... can be removed from expectedNpmVersionFailures.txt in ..." will be removed from expectedNpmVersionFailures.txt, as the warning suggests.

It may be possible to integrate these scripts into DT(-tools), to set up regular job for updating expectedNpmVersionFailures.txt in DT-tools, but it will need DT(-tools)'s maintainers' comment on how to properly do so.

[jakebailey]

It's certainly possible to have a GHA workflow here which just runs ATTW only on all of DT on a weekly basis or something; I believe we have a flag for that, and I don't even think it requires DT have done a pnpm install (might be wrong).

If you had this script in dt-tools, and then a workflow which ran the script then verified that DT passes and then errors out with a patch if there's any diff, that would probably be useful.

As an example, https://github.com/microsoft/TypeScript/blob/700ee076e515db2ef49d8cf7e4dc4bf70679575c/.github/workflows/ci.yml#L312-L340 more or less does something similar, though it's "overcomplicated" and its diff could just be git diff --staged --exit-code or something like https://github.com/microsoft/TypeScript/blob/700ee076e515db2ef49d8cf7e4dc4bf70679575c/.github/workflows/ci.yml#L70.

[jakebailey]

Or better, do what https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/.github/workflows/ghostbuster.yml does and have a PR sent to the repo.

[jakebailey]

I manually checked all of these against the DT logs. These ones did not fail with the same warning:

ignore-by-default
passport-weibo
resolve-dir
react-svg-pan-zoom-loader

But with Ignoring npm version error because ... was failing when the check was added; I assume these packages fail both checks and DefinitelyTyped/DefinitelyTyped#71620 would change their warnings? (I don't see the new warnings in those logs either, so maybe this is a case of one check not running in attw.json modifications.)

[hkleungai]

You are right. I just locally re-run my script and find that these 4 should not get removed.

Let me try to write script to let github action do the job instead.