fix(ci): green node:typecheck for plugins-registry and feature-flags sdk - #93
Merged
Merged
Conversation
…re-flags sdk nself-ci's new implicit-workspace-member discovery (gate_workspace.go, #91) found and ran node:typecheck against .workers/plugins-registry and free/feature-flags/sdk-ts for the first time, and both failed on main. .workers/plugins-registry already declared @cloudflare/workers-types correctly in package.json/tsconfig and had a valid committed lockfile — the TS2688 only happened because nself-ci.yml never installed this directory's dependencies before running its typecheck script, so tsc fell back to the runner's global toolchain with no project node_modules at all. Same root cause produced the sdk-ts TS2307 on 'react'. Added an install step for both nested packages (this repo has no pnpm-workspace.yaml, so pnpm never recurses into them on its own) and committed a lockfile for sdk-ts, which had none. Separately, sdk-ts's evaluate() typed its ctx parameter as the full EvaluateRequest even though flag_key is always supplied by the explicit key argument, never by ctx — evaluateFlag()/useFlag() correctly pass Omit<EvaluateRequest, 'flag_key'>, which the wider parameter type rejected (TS2345). Narrowed evaluate()'s ctx type to match what it actually needs, and swapped the request-body spread order so an explicit key can never be silently overridden by a flag_key on ctx. free/file-processing/ts, free/media-processing/ts, and the sdk-ts jest config remain red for unrelated pre-existing reasons and are out of scope here.
2 tasks
…e jest transform for feature-flags sdk (#94) nself-ci's node gate surfaced three more pre-existing reds beyond PR #93's scope (same root cause class: nothing installs a nested package's own dependencies before typecheck/test/build runs): - free/file-processing/ts and free/media-processing/ts both had every declared dependency (commander, fastify, @types/node, @nself/plugin-utils, etc.) present in package.json but never installed. Added the same `pnpm install --frozen-lockfile --dir <pkg>` pattern PR #93 introduced. Their @nself/plugin-utils dependency is a file: path to shared/, whose dist/ output pnpm never builds on link — added a build step for shared ahead of the gate so the type/JS output exists. - Installing file-processing surfaced a genuine bug once tsc could actually resolve @nself/plugin-utils: src/server.ts imports createMetrics, which was never implemented in the shared package. Added a minimal dependency-free Prometheus-text-format counter (shared/src/metrics.ts) matching the incrementRequest/incrementError/format surface the plugin already calls. - free/feature-flags/sdk-ts had ts-jest installed but no jest config at all, so jest ran with zero TypeScript transform. Added jest.config.js (ts-jest preset + a moduleNameMapper for the NodeNext-style `.js` import extensions). That surfaced two more real gaps: the tsconfig's Node16/NodeNext module kind needs isolatedModules for ts-jest's per-file transpile, and the test file's `global.fetch` mock needed @types/node (never a declared dependency here). scripts/nself-ci.sh --check --no-gitleaks -v . now reports PASS for every node:typecheck, node:test, and node:build entry across all 5 discovered workspace members.
The nself-ci gate still failed with 'Cannot find module @nself/plugin-utils' across every importing file in free/file-processing/ts and free/media-processing/ts, even though shared was installed and built. Cause: pnpm does not symlink a file: dependency back to the source tree. It takes a real directory COPY into its store at install time. Building shared afterwards populates shared/dist in the source tree but not in the copy the dependents resolve against, and @nself/plugin-utils' main/types point at ./dist/*, so tsc finds no module. Reordered so shared is built before file-processing and media-processing are installed, which means their copies already contain dist/. Verified from a clean state (dist and node_modules removed): both typechecks pass. No gate was weakened and no dependency changed - only step ordering.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
node:typecheckagainst.workers/plugins-registryandfree/feature-flags/sdk-tsfor the first time, and both failed, turning main red..workers/plugins-registryalready declared@cloudflare/workers-typescorrectly and had a valid lockfile — the TS2688 happened becausenself-ci.ymlnever installed this directory's own dependencies before typechecking it (nopnpm-workspace.yamlexists in this repo, so pnpm never recurses into nested packages on its own). Added an explicit install step for it; no source change needed.free/feature-flags/sdk-tshit the same missing-install cause for its TS2307 onreact, plus a genuine type bug (TS2345):evaluate()typed itsctxparameter as the fullEvaluateRequest, butflag_keyis always supplied by the explicitkeyargument, never byctx—evaluateFlag()/useFlag()correctly passOmit<EvaluateRequest, 'flag_key'>, which didn't satisfy the wider parameter type. Narrowedevaluate()'sctxtype to match what it actually needs, and putflag_key: keylast in the request-body spread so an explicit key can never be silently overridden by aflag_keyonctx. Also committed apnpm-lock.yamlfor this package, which had none.Out of scope (still red, pre-existing, unrelated)
free/file-processing/tsandfree/media-processing/ts:node:typecheck/node:buildfail on missingcommander/@nself/plugin-utils/@types/node, andnode:teston a missingfastify— this needs its own dependency/workspace-linking fix.free/feature-flags/sdk-tsnode:test: jest has no babel/ts-jest config wired up, so it can't parse.tstest files — separate from the typecheck fix here.Test plan
pnpm install --frozen-lockfile --dir .workers/plugins-registry && pnpm run typecheck— PASSpnpm install --frozen-lockfile --dir free/feature-flags/sdk-ts && pnpm run typecheck— PASSpnpm run buildinfree/feature-flags/sdk-ts— PASSscripts/nself-ci.sh --check --no-gitleaks -v .locally —node:typecheck (.workers/plugins-registry)andnode:typecheck (free/feature-flags/sdk-ts)both PASS (remaining FAILs are the pre-existing, unrelated items noted above)