Conversation
Reversean
force-pushed
the
refactor/hawk-storage
branch
3 times, most recently
from
c3013b6 to
223001d
Compare
Reversean
changed the base branch from
chore/browser-workspace
to
chore/core-workspace
There was a problem hiding this comment.
Pull request overview
Adds an environment-agnostic storage abstraction to @hawk.so/core and introduces a browser localStorage-backed implementation in @hawk.so/javascript, aligning with the repo’s ongoing split between shared core logic and runtime-specific implementations (Issue #151).
Changes:
- Added
HawkStorageinterface to@hawk.so/coreand exported it from the core entrypoint. - Added
HawkLocalStorageimplementation and corresponding unit tests in@hawk.so/javascript. - Added Vitest coverage support/config updates for the JavaScript package.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Adds dependencies for Vitest v8 coverage reporting. |
| packages/javascript/tsconfig.test.json | Adds Vitest globals types to the test TS config. |
| packages/javascript/tests/storages/hawk-local-storage.test.ts | Introduces unit tests for the new HawkLocalStorage. |
| packages/javascript/src/storages/hawk-local-storage.ts | Adds a localStorage-backed HawkStorage implementation. |
| packages/javascript/package.json | Adds test:coverage script and coverage dependency. |
| packages/core/src/storages/hawk-storage.ts | Adds the storage interface contract in core. |
| packages/core/src/index.ts | Exports HawkStorage from the core public entrypoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "error-stack-parser": "^2.1.4" | ||
| }, | ||
| "devDependencies": { | ||
| "@hawk.so/types": "0.5.8", | ||
| "@vitest/coverage-v8": "^4.0.18", | ||
| "jsdom": "^28.0.0", | ||
| "vite": "^7.3.1", |
There was a problem hiding this comment.
HawkLocalStorage imports @hawk.so/core, but @hawk.so/javascript does not declare @hawk.so/core as a dependency/devDependency. With Yarn PnP/workspaces this will fail module resolution and break tests/build. Add @hawk.so/core to this package’s dependencies (typically "workspace:*").
There was a problem hiding this comment.
Added @hawk.so/core as dependency
| * @param key - Storage key to look up. | ||
| */ | ||
| getItem(key: string): string | null | ||
|
|
||
| /** | ||
| * Persists a value under the given key. | ||
| * | ||
| * @param key - Storage key. | ||
| * @param value - Value to store. | ||
| */ | ||
| setItem(key: string, value: string): void | ||
|
|
||
| /** | ||
| * Removes the entry for the given key. | ||
| * | ||
| * @param key - Storage key to remove. | ||
| */ | ||
| removeItem(key: string): void | ||
| } |
There was a problem hiding this comment.
This interface’s method signatures are missing the delimiter semicolons used elsewhere in the repo’s TypeScript interfaces. This is likely to violate the project ESLint/TS style rules (e.g., member-delimiter-style / semi) and should be updated to match existing interfaces.
| import { beforeEach, afterEach, describe, it, expect, vi, Mock } from "vitest"; | ||
| import { HawkLocalStorage } from "../../src/storages/hawk-local-storage"; | ||
|
|
||
| describe('HawkLocalStorage', () => { | ||
| let getItemSpy: Mock<(key: string) => string | null>; | ||
| let setItemSpy: Mock<(key: string, value: string) => void>; | ||
| let removeItemSpy: Mock<(key: string) => void>; |
There was a problem hiding this comment.
Mock is only used as a type here, but it’s imported as a runtime named import from vitest. Prefer importing it as a type-only import (or use ReturnType<typeof vi.spyOn> / MockInstance) to avoid requiring a runtime Mock export and to keep the emitted JS clean.
There was a problem hiding this comment.
Fixed, now importing Mock as type.
Not sure if usage of ReturnType is better sollution since it resolved as any type, and not sure if declaring mocks as const wont break anything.
| import { beforeEach, afterEach, describe, it, expect, vi, Mock } from "vitest"; | ||
| import { HawkLocalStorage } from "../../src/storages/hawk-local-storage"; |
There was a problem hiding this comment.
This new test uses double quotes in imports, while existing tests in this package consistently use single quotes. Aligning with the established quote style will avoid ESLint churn.
| import { beforeEach, afterEach, describe, it, expect, vi, Mock } from "vitest"; | |
| import { HawkLocalStorage } from "../../src/storages/hawk-local-storage"; | |
| import { beforeEach, afterEach, describe, it, expect, vi, Mock } from 'vitest'; | |
| import { HawkLocalStorage } from '../../src/storages/hawk-local-storage'; |
Reversean
force-pushed
the
refactor/hawk-storage
branch
2 times, most recently
from
8cbdac2 to
cd47f1e
Compare
2 participants
Related #151
Added
HawkStorageinterface which could be used as env-agnositc abstraction to store key-values.Also
HawkLocalStorageimplementation based on browserlocalStorageadded.