fix: detect implicit types in package timeline - #3102
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Hello! Thank you for opening your first PR to npmx, @anilloutombam! 🚀 Here’s what will happen next:
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe timeline endpoint analyses package contents for eligible versions to detect declaration files. It returns enriched ChangesTimeline type detection
Sequence Diagram(s)sequenceDiagram
participant TimelineEndpoint
participant PackageFetcher
participant PackageAnalyzer
TimelineEndpoint->>PackageFetcher: Fetch package files and type-package data
PackageFetcher-->>TimelineEndpoint: Return package contents
TimelineEndpoint->>PackageAnalyzer: Analyse package contents
PackageAnalyzer-->>TimelineEndpoint: Return included-type result
TimelineEndpoint-->>TimelineEndpoint: Set hasTypes on the visible page
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/api/registry/timeline/`[...pkg].get.ts:
- Around line 87-92: Update possibleTypeRemovals in
server/api/registry/timeline/[...pkg].get.ts (lines 87-92) to include every
consecutive untyped version following the metadata transition, so each affected
version is analyzed and retains hasTypes: true when implicit declaration files
exist. Extend the corresponding test scenario in
test/unit/server/api/registry/timeline/pkg.get.spec.ts (lines 232-266) with a
third version containing an implicit declaration file and assert both
post-transition versions return hasTypes: true.
In `@test/unit/server/api/registry/timeline/pkg.get.spec.ts`:
- Around line 264-265: Update the assertions in the versions test to avoid
non-null assertions on result.versions indices; use optional access or
explicitly validate the indexed version objects before reading hasTypes, while
preserving the expected true assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f09bad7-d2db-498f-be6e-6f84c519bedc
📒 Files selected for processing (2)
server/api/registry/timeline/[...pkg].get.tstest/unit/server/api/registry/timeline/pkg.get.spec.ts
| const possibleTypeRemovals = visibleVersions | ||
| .filter((version, index) => { | ||
| if (version.hasTypes) return false | ||
| return allVersions.slice(index + 1).some(previousVersion => previousVersion.hasTypes) |
There was a problem hiding this comment.
Could you explain the thinking here a bit? I guess it's so that you don't need to fetch a bunch of data for every package to check the types? Do you think it may miss packages that add types in weird ways? I wonder if instead we should run the fetch logic in the main allVersions loop if hasBuiltInTypes fails 🤔 I guess that would be less efficient... maybe you could add a comment explaining the rationale?
There was a problem hiding this comment.
Yes, that was the thinking. The file-aware check requires extra package and file-tree requests, so I limited it to metadata-untyped versions that have an older typed release. Those are the versions that could otherwise produce a false “types removed” event.
This could miss a package that has always shipped types implicitly, or adds them without using types or typings. Detecting those would require checking every metadata-untyped version, which is broader than this removal-event fix and would add more requests.
I added a comment explaining the tradeoff. I’m happy to broaden the check if you think the extra requests are acceptable.
| .filter(version => { | ||
| if (version.hasTypes) return false | ||
|
|
||
| const versionIndex = allVersions.indexOf(version) |
There was a problem hiding this comment.
I changed this to use the index from filter earlier, any reason that was undone?
There was a problem hiding this comment.
Yeah, that was intentional. visibleVersions.filter() gives us a page-relative index, but we’re slicing allVersions. That breaks as soon as the offset is non-zero. Using allVersions.indexOf(version) gives us the version’s actual position in the full timeline.
ghostdevv
added
the
needs review
2 participants
🔗 Linked issue
Fixes #2791
🧭 Context
The timeline incorrectly showed removed TypeScript types when declarations still existed as
.d.mtsfiles beside.mjsentry points.📚 Description
I updated the timeline to check the published package files when types appear to be removed. I also added a regression test for declarations stored beside the package entry points.