Conversation
Published scrolloop@0.5.1 declared unresolvable deps (@scrolloop/core, @scrolloop/shared are not on npm) so npm install scrolloop failed. Force-bundle them via tsup noExternal and duplicate the two shared interfaces in the adapter packages so the public .d.ts is self-contained. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📊 Test Coverage Report (vitest)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request increments the version to 0.5.2, removes internal workspace dependencies from the root package.json, and redefines the Range and PageResponse interfaces locally within the React and React Native packages. The tsup configuration is updated to bundle these internal dependencies. Feedback suggests that duplicating these interfaces introduces maintenance risks and recommends using build-time inlining to maintain a single source of truth.
| export interface Range { | ||
| startIndex: number; | ||
| endIndex: number; | ||
| } | ||
|
|
||
| export interface PageResponse<T> { | ||
| items: T[]; | ||
| total: number; | ||
| hasMore: boolean; | ||
| } |
There was a problem hiding this comment.
Defining these interfaces locally instead of importing them from @scrolloop/shared introduces a maintenance risk where the definitions might diverge over time. If the tsup configuration can be adjusted to reliably inline these types into the .d.ts file (using explicit dts.resolve), it would be preferable to keep the single source of truth in the shared package.
| export interface Range { | ||
| startIndex: number; | ||
| endIndex: number; | ||
| } | ||
|
|
||
| export interface PageResponse<T> { | ||
| items: T[]; | ||
| total: number; | ||
| hasMore: boolean; | ||
| } |
There was a problem hiding this comment.
As with the React package, this duplication is a potential source of bugs if the shared types are updated but these local copies are forgotten. Relying on build-time inlining of types is a safer long-term approach for maintaining a DRY codebase.
1 participant
Summary
scrolloop@0.5.1declared@scrolloop/coreand@scrolloop/sharedas runtime deps, but those packages aren't on npm —npm install scrolloopfails to resolve them.noExternal, and duplicateRange/PageResponseinterfaces in the adapter packages so the public.d.tsis also self-contained (tsupdts.resolvewasn't inlining workspace package types reliably).0.5.2(0.5.1 on npm is immutable and broken).Test plan
pnpm run build— all 6 packages build successfullypnpm run typecheck— passespnpm run test— 21 tests passdist/index.{cjs,mjs,d.ts,d.cts}contain zero references to@scrolloop/*after buildnpm packtarball has no runtimedependencies, onlypeerDependencies: { react: '>=18.0.0' }scrolloop@0.5.2on npm is installable standalone🤖 Generated with Claude Code