fix(npm): handle boolean-form deprecated field - #48
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the npm registry client’s robustness by allowing FetchVersions / FetchPackage to decode npm packuments where a version’s deprecated field is emitted as a boolean (a real-world deviation from the spec), preventing JSON unmarshal failures for packages like react.
Changes:
- Introduces a
deprecatedFieldtype with customUnmarshalJSONto normalizedeprecatedvalues across string/boolean/null shapes. - Updates
versionInfo.Deprecatedto usedeprecatedFieldso packument decoding no longer fails on boolean forms. - Adds a unit test covering multiple
deprecatedshapes and asserting resultingVersion.Statusand metadata.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/npm/npm.go | Adds deprecatedField JSON decoding to accept boolean/null and normalizes to a string-like value used by existing deprecation logic. |
| internal/npm/npm_test.go | Adds test coverage for deprecated field shape variations during FetchVersions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+165
to
+169
| // TestFetchVersions_DeprecatedShapes verifies that the "deprecated" field is | ||
| // handled across the shapes npm packuments emit: absent/null, a string | ||
| // message, and the legacy boolean form (false == not deprecated, | ||
| // true == deprecated with no message). String and true values must mark the | ||
| // version StatusDeprecated; absent/null/false must leave it active. |
Comment on lines
+239
to
+241
| if got := string(v.Metadata["deprecated"].(deprecatedField)); got != wantDep[num] { | ||
| t.Errorf("%s deprecated metadata = %q, want %q", num, got, wantDep[num]) | ||
| } |
andrew
requested changes
Jul 26, 2026
andrew
left a comment
Contributor
There was a problem hiding this comment.
Thanks for this — the fix and test coverage look good.
Two things before merge:
- This now conflicts with main after #47 landed (both PRs insert tests at the same spot in
npm_test.goand touch adjacent lines in theversionInfostruct). Could you rebase? - Optional: at the
Metadataassignment, consider"deprecated": string(v.Deprecated)so the map keeps holding a plainstringas before rather than the nameddeprecatedFieldtype. Nothing currently type-asserts it, so not blocking.
The npm packument spec defines a version's `deprecated` field as a string carrying the deprecation message, but some packuments emit a boolean instead (false meaning "not deprecated", true meaning deprecated with no message). The struct typed it as `string`, so unmarshalling failed with "cannot unmarshal bool into Go struct field versionInfo.versions.deprecated of type string" and FetchVersions returned an error for the whole package (e.g. react, which has 5 versions with `deprecated: false`). Introduce a `deprecatedField` string type with a custom UnmarshalJSON that accepts a string, boolean, or null and normalizes them to the string form the rest of the code relies on. Strings are kept verbatim; `true` maps to "true" (deprecated); `false` and null map to "" (not deprecated). This preserves the existing non-empty check that drives StatusDeprecated, so boolean-false versions stay active while string/true versions stay deprecated. Adds TestFetchVersions_DeprecatedShapes covering absent, string-message, boolean-false, and boolean-true forms.
acidghost
force-pushed
the
fix/npm-deprecated-boolean
branch
from
July 26, 2026 18:31
3407fc1 to
2bcf06a
Compare
andrew
approved these changes
Jul 27, 2026
andrew
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the rebase and the metadata cast — LGTM.
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
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.
Problem
FetchVersions/FetchPackagefail with a JSON unmarshal error for packages whose versions publishdeprecatedas a boolean instead of a string:reacttriggers this — several of its versions publishdeprecated: false.Reproducer
returns:
{"version":"16.7.0","deprecated":false} {"version":"16.8.0-alpha.0","deprecated":false} {"version":"0.14.10","deprecated":false} {"version":"15.7.0","deprecated":false} {"version":"16.14.0","deprecated":false}so
FetchVersions(ctx, "react")errors out instead of returning versions.Root cause
The npm packument spec defines
deprecatedas a string (the deprecation message), but some packuments emit a boolean instead (false= "not deprecated",true= "deprecated with no message").versionInfo.Deprecatedis typedstring, so the boolean form fails the entirepackageResponsedecode.Unlike the other polymorphic npm fields,
deprecateddrives logic:so simply switching to
interface{}is not enough.Fix
Introduce a
deprecatedFieldstring type with a customUnmarshalJSONthat accepts a string, boolean, or null and normalizes to the string form the existing logic expects:StatusDeprecated)true→"true"(deprecated)false/null→""(not deprecated)This preserves the non-empty check driving
StatusDeprecated, so boolean-falseversions stay active while string /trueversions stay deprecated.Testing
Added
TestFetchVersions_DeprecatedShapescovering absent, string-message, boolean-false, and boolean-trueforms, asserting both the resultingVersion.Statusand theVersion.Metadata["deprecated"]value.