WIP: Extend optional functions usage verification - #541
Draft
MatiasFernandez wants to merge 3 commits into
Draft
Conversation
MatiasFernandez
force-pushed
the
mfernandez/migrate-gci-tests-phase-6c
branch
from
September 7, 2026 18:56
c37b04d to
1db0b1c
Compare
Every doc that describes this chain asserted the old design by name, down to a test file that no longer exists. The enforcement table's registry-vs-headers rows are now a CI step rather than a unit test, and "record a wrong floor" isn't a mistake you can make by hand any more. Rewrote "Why the registry is hand-written rather than generated" instead of deleting it. Both of its arguments — the floor visible in the diff, a moving floor staying loud — are what committing the generated file is built to satisfy, and dropping the section would read as the argument having been lost rather than answered. Its third point, that vendor/** never ships, is satisfied for the first time now that a .ts does. Both READMEs' "adding a version" steps change from "run npm test and read the failure" to "regenerate, then compile", because npm test alone no longer sees a stale registry — that gap is written up under the enforcement table rather than left in the commit history, since it is the one thing a developer actually walks into. Also drops gciVersionGated's claim that eslint.config.mjs can't import TypeScript. It can, via Node type-stripping, which is why optionalFunctions.ts is exempted from the repo-wide .ts-import ban; leaving the comment would strand the next reader on a false constraint.
gciVersionGated.test.ts gated only on `addedIn`, so production code calling a Windows-absent (`GciTsDebugConnectToGem`, `GciTsNbLogin_`) or 4.0-removed (`GciTsEncrypt`) symbol passed lint, passed `npm test` and passed every Linux cell, then threw on the five windows-latest cells — or in a user's Windows install, if the call site sat behind a flag no Windows test exercises. The gated set is now every key of `GCI_OPTIONAL_FUNCTIONS`, so the test states the real rule: production should not call any registry symbol. Renamed to gciOptionalityGate.test.ts, since its subject is no longer the 3.6.2 floor. One unified set rather than the second set and second allowlist originally sketched — an extracted `hazardFor(name)` builds the message from the entry's own fields, reporting both clauses for a two-axis symbol, which is all the split bought. ALLOWED_POST_362 becomes ALLOWED_OPTIONAL_FUNCTION and grows from one name to three: the pre-existing guarded call sites in nbRunner.ts and sessionManager.ts, so the widened gate lands at zero new offenders. Its comment now says the list must not grow — a new entry reads like a reviewable opt-in but isn't one, since it accepts a cross-version conditional above the binding layer. The intended answer is a helper in client/src/gciLibrary/, which would let the allowlist be deleted; relocating those two conditionals is left as a follow-up.
The deleted test's own header claimed a lint rule would have to restate the gated names, because eslint.config.mjs can't import TypeScript. Node 24 strips types, so it can: the config imports optionalFunctions.ts directly and generates one no-restricted-syntax selector per registry entry, carrying the same per-axis hazard message. Nothing the test did lost an enforcer. Its 40 lines of readdirSync and basename filtering become the block's `files`/`ignores`. Its ALLOWED_OPTIONAL_FUNCTION list becomes an eslint-disable-next-line at each call site, whose `-- reason` is forced by eslint-comments/require-description. Its stale-entry test becomes reportUnusedDisableDirectives, already on — a disable outliving its call site fails lint. And AST selectors are stricter than the old `\.<name>\s*\(` scan: they also catch gci['GciTsNbPoll'](...) and ignore the name inside a comment or string. Generated selectors rather than a custom rule in a local plugin, which reads better and could grow to understand the isAvailable guard. ESLint hashes each file against its resolved config (lint-result-cache.js:56), which sees rule options but not data a rule closed over — so a rule reading the registry internally would leave already-cached files green after a new entry lands. Options keep the cache honest. eslint.config.ts was the other route; ESLint 10 hard-errors without jiti, and Node's own type stripping already does that job. Two things genuinely change. The exemption now sits at the call site, where a reader of nbRunner.ts sees it, but is also cheaper to add than an entry under a "must not grow" comment — eslint-comments/ no-restricted-disable can ban it later if that deterrent is wanted back. And enforcement moves from `npm test` to CI's lint job, still a required gate, now also visible in the editor on the offending line. The one cost is silencing MODULE_TYPELESS_PACKAGE_JSON, which Node raises because client/package.json declares no module type: NODE_OPTIONS=--disable-warning=MODULE_TYPELESS_PACKAGE_JSON on the lint scripts and the lefthook hook. Set through cross-env, a devDependency, because a bare VAR=value prefix is Unix shell syntax — under cmd.exe it is a command name, so without it `npm run lint` and the pre-commit hook are both broken for a Windows contributor. Node's advice in the warning itself is not an option: "type": "module" in client/package.json would reinterpret client/out/extension.js, the tsc-emitted CommonJS the extension loads as `main`, along with every tracked client/src/*.js.
MatiasFernandez
force-pushed
the
mfernandez/migrate-gci-tests-phase-6c
branch
from
September 7, 2026 20:38
1db0b1c to
38d0ce6
Compare
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.
No description provided.