Skip to content

Fix: declare @types/js-yaml in core/package.json devDependencies - #680

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/core-types-js-yaml-devdependency
Open

Fix: declare @types/js-yaml in core/package.json devDependencies#680
AmaadMartin wants to merge 1 commit into
mainfrom
fix/core-types-js-yaml-devdependency

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

N/A

  1. Or, if no issue exists, describe the change:

Problem: core/src imports js-yaml in two files, and js-yaml ships no bundled type declarations. @types/js-yaml is declared only in the root manifest, so npm workspace hoisting is the only reason core typechecks today. core/package.json therefore does not declare everything core needs in order to build.

Solution: I declared @types/js-yaml in core/package.json devDependencies. The range ^4.0.9 is copied verbatim from the root manifest, so npm resolves both declarations to the one existing tree and no second copy enters node_modules. It is a devDependency because no js-yaml type reaches core/dist/types: yaml.load returns unknown in @types/js-yaml v4, and both call sites consume that unknown immediately. This follows the precedent already in the same manifest, @types/adm-zip.

The root declaration stays. dev/src also imports js-yaml and dev/package.json does not declare the types yet, so removing the root entry would break dev.

Collision check: I listed all 379 open pull requests on the fork and diffed the 9 that touch core/package.json (#273, #291, #292, #323, #327, #445, #541, #544, #635). None adds @types/js-yaml. #273, #323 and #541 move openapi-types out of the same block and will conflict textually if one lands first; the resolution is to keep both edits.

Testing Plan

This change adds no executable code, so it adds no test. The verification is the manifest reproduction, run in both directions.

Proof the change is load-bearing. The mutation is to delete the root declaration, which removes the hoisting accident.

Before the fix, npx tsc --noEmit -p core/tsconfig.json fails:

core/src/skills/loader.ts:8:18 - error TS7016: Could not find a declaration file for module 'js-yaml'.
core/src/tools/openapi_tool/openapi_toolset.ts:7:18 - error TS7016: Could not find a declaration file for module 'js-yaml'.
Found 2 errors in 2 files.

After the fix, with the root declaration still deleted, the same command exits 0. Core's own declaration supplies the types. I then restored the root manifest, so the final diff is core/package.json and package-lock.json only.

Unit Tests:

  • I have added or updated unit tests for my change. (None added: no new executable lines. The two existing files that run yaml.load are the regression net.)
  • All unit tests pass locally.

npx vitest run core/test/skills/loader_test.ts core/test/tools/openapi_tool/openapi_toolset_integration_test.ts --project unit:core — 49 passed.

npx vitest run --project unit:core --project unit:dev — 2583 passed, 1 failed. The failure is dev/test/cli/cli_create_test.ts > should handle Vertex AI selection with gcloud defaults. It reads local gcloud defaults and fails identically on the unmodified tree, so it is unrelated to this change.

Other gates, all from the repository root:

  • npm install — changes only the two files above. The lockfile diff is one added line in packages.core.devDependencies.
  • npm run build — succeeds.
  • grep -rn "js-yaml" core/dist/types/ — no match, so no type leaks into the published declarations.
  • npm run lint, npm run format:check, npx secretlint "**/*" — clean.
  • npm run ts:check — 281 errors, the same count before and after this change. They are pre-existing and unrelated.

Manual End-to-End (E2E) Tests:

Confirm the entry cannot reach consumers:

npm pack --dry-run --workspace core

It lists package.json, README.md, LICENSE and dist/** only, with no @types payload.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

core/src imports js-yaml from two files, but js-yaml ships no bundled
type declarations. @types/js-yaml was declared only in the workspace
root manifest, and npm workspace hoisting resolved it into core. Delete
the root declaration and core stops typechecking with two TS7016 errors.

The range ^4.0.9 is copied verbatim from the root manifest, so npm
resolves both declarations to the one existing tree and no second copy
of the type package enters node_modules.

It is a devDependency, not a dependency, because no js-yaml type reaches
core/dist/types: yaml.load returns unknown in @types/js-yaml v4, and both
call sites consume that unknown immediately. This follows the precedent
already set in the same manifest by @types/adm-zip.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant