amoghrajesh commented on code in PR #71399: URL: https://github.com/apache/airflow/pull/71399#discussion_r3860004492
########## ts-sdk/scripts/verify-package.mjs: ########## @@ -0,0 +1,154 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { spawnSync } from "node:child_process"; + +// A packaging check must never be the reason a static-check run hangs: npm install reaches +// the registry, and pnpm pack shells out to a full TypeScript build. +const COMMAND_TIMEOUT_MS = 10 * 60 * 1000; + +const requiredRootFiles = ["LICENSE", "NOTICE", "README.md", "package.json"]; + +function run(command, args, options = {}) { + const result = spawnSync(command, args, { + encoding: "utf8", + timeout: COMMAND_TIMEOUT_MS, + maxBuffer: 16 * 1024 * 1024, + ...options, + }); + if (result.error) { + throw new Error(`${command} ${args.join(" ")} failed: ${result.error.message}`, { + cause: result.error, + }); + } + if (result.status !== 0) { + process.stderr.write(result.stdout ?? ""); + process.stderr.write(result.stderr ?? ""); + throw new Error(`${command} ${args.join(" ")} failed with exit code ${result.status}`); + } + return result; +} + +function isAllowedFile(path) { + return requiredRootFiles.includes(path) || /^dist\/.+\.(?:js|d\.ts)$/.test(path); +} + +/** + * Every path a consumer can resolve — the `exports` conditions plus the `bin` targets. Derived + * from package.json so adding an export subpath extends the check instead of silently escaping it. + */ +function collectEntryPoints(packageJson) { + const targets = Object.values(packageJson.exports ?? {}).flatMap((conditions) => + typeof conditions === "string" ? [conditions] : Object.values(conditions), + ); + targets.push(...Object.values(packageJson.bin ?? {})); + return targets.map((target) => target.replace(/^\.\//, "")); +} + +const temporaryDirectory = mkdtempSync(join(tmpdir(), "airflow-ts-sdk-package-")); + +try { + const packageJson = JSON.parse(readFileSync("package.json", "utf8")); + const requiredFiles = [...requiredRootFiles, ...collectEntryPoints(packageJson)]; + + const packed = run("pnpm", [ + "--silent", + "pack", + "--json", + "--pack-destination", + temporaryDirectory, + ]); + // `--silent` does not suppress the `prepack` lifecycle banner, so stdout is build log followed + // by the JSON report — take the trailing object rather than parsing the whole stream. + const metadataMatch = packed.stdout.match(/(?:^|\n)(\{[\s\S]*\})\s*$/); + if (!metadataMatch) { + throw new Error("pnpm pack did not return package metadata"); + } + const metadata = JSON.parse(metadataMatch[1]); + if (!Array.isArray(metadata.files) || typeof metadata.filename !== "string") { + throw new Error("pnpm pack returned invalid package metadata"); + } + const paths = new Set(metadata.files.map(({ path }) => path)); + + const missing = requiredFiles.filter((path) => !paths.has(path)); + const unexpected = [...paths].filter((path) => !isAllowedFile(path)); + if (missing.length > 0 || unexpected.length > 0) { + throw new Error( + [ + missing.length > 0 ? `missing required files: ${missing.join(", ")}` : "", + unexpected.length > 0 ? `unexpected files: ${unexpected.join(", ")}` : "", + ] + .filter(Boolean) + .join("; "), + ); + } + + if (metadata.name !== packageJson.name || metadata.version !== packageJson.version) { + throw new Error("packed package identity does not match package.json"); + } + + const consumerDirectory = join(temporaryDirectory, "consumer"); + mkdirSync(consumerDirectory); + writeFileSync( + join(consumerDirectory, "package.json"), + JSON.stringify({ name: "airflow-ts-sdk-package-smoke-test", private: true, type: "module" }), + ); + run("npm", ["install", "--ignore-scripts", "--no-audit", "--no-fund", metadata.filename], { + cwd: consumerDirectory, + }); + run( + "node", + [ + "--input-type=module", + "--eval", + `const sdk = await import(${JSON.stringify(packageJson.name)}); if (typeof sdk.registerTask !== "function") process.exit(1);`, Review Comment: I don't see `registerTask` defined. I think its `registerDags` / `serveDags` now? ########## ts-sdk/README.md: ########## @@ -207,9 +208,21 @@ can enter the lockfile, transitive dependencies cannot use Git or arbitrary tarball sources, and only explicitly approved dependencies can run lifecycle build scripts. Review changes to both files together when updating dependencies. +`verify:package` creates the npm tarball, rejects files outside the published +runtime allowlist, installs it into a clean temporary project, and smoke-tests +every `exports` entry point and the `bin` executable. The required paths are Review Comment: The code imports only the package root (`verify-package.mjs:108-113`, `await import(packageJson.name)`). The `./coordinator` subpath is never imported; `collectEntryPoints` only puts `dist/coordinator/index.js` and `dist/coordinator/index.d.ts` into the presence list. Presence is not resolution, a subpath can be present and still fail to import if exports is misspelled or an internal import is broken. Either fix the sentence to say the entry points are checked for presence and the root is imported, or loop the import over the exports keys. The second is better and is a small change, since collectEntryPoints already walks exports. ########## ts-sdk/.pre-commit-config.yaml: ########## @@ -64,3 +64,19 @@ repos: additional_dependencies: ['[email protected]'] pass_filenames: false require_serial: true + - id: verify-ts-sdk-package + name: Verify TypeScript SDK package artifact + entry: ./scripts/ci/prek/verify_ts_sdk_package.py + language: node + stages: [manual] + files: | + (?x) + ^src/.*\.ts$| + ^scripts/verify-package\.mjs$| + ^package\.json$| + ^pnpm-lock\.yaml$| + ^pnpm-workspace\.yaml$| + ^tsconfig(\.build)?\.json$ + additional_dependencies: ['[email protected]'] + pass_filenames: false + require_serial: true Review Comment: Nothing runs this check automatically, so it does not yet do what the PR body says. The hook is `stages: [manual]`, CI runs no ts-sdk manual hooks, and the release workflow proposed in #71843 runs `pnpm run lint, format:check, typecheck, and test` before `npm pack` - not `verify:package`. A contributor has to remember to run it, and the one time it mattered nobody did. The natural home is #71843 based build job, one line before npm pack. That is also where it has the most value: it would verify the exact tarball that gets published. Since both PRs are open and from the same author, they can be sequenced. ########## ts-sdk/scripts/verify-package.mjs: ########## Review Comment: has no tests -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
