Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the repo’s tooling (switching from Yarn to npm and updating dev tooling deps) and extends the Keybinding component to support scoping listeners via a React RefObject target.
Changes:
- Migrate from Yarn to npm (
yarn.lockremoved,package-lock.jsonadded; CI and scripts updated). - Add
RefObject<HTMLElement>support fortargetinKeybinding, with updated target resolution logic. - Upgrade devDependencies (Biome, TypeScript, React typings) and update Biome configuration schema/settings.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Removed as part of moving away from Yarn. |
| package-lock.json | Added to lock npm dependency resolution for CI/releases. |
| package.json | Bumped version, updated scripts to npm, upgraded devDependencies. |
| .github/workflows/ci.yml | Switched CI install/build/test steps from Yarn to npm. |
| biome.json | Updated Biome schema version and file include/exclude configuration. |
| src/keybinding.tsx | Added RefObject target support and adjusted key dispatch/conflict logic. |
| README.md | Documented RefObject usage for scoping listeners to a container. |
| .vscode/settings.json | Added editor defaults for Biome formatting-on-save. |
| .prettierignore | Removed (no longer used). |
| .eslintrc / .eslintignore | Removed (Biomify migration). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "prepublishOnly": "npm run publint && npm run attw -P && npm run build", | ||
| "build": "tsup src/keybinding.tsx --format esm,cjs --dts --sourcemap --external react", | ||
| "test:typings": "tsc --noEmit --project ./tsconfig.json", | ||
| "test:lint": "biome ci .", | ||
| "test:lint:fix": "biome check --apply ." | ||
| "test:lint:fix": "biome check --apply .", | ||
| "publint": "publint", | ||
| "attw": " attw -P" | ||
| }, |
There was a problem hiding this comment.
npm run attw -P won’t pass -P to the script (npm treats it as an npm option unless you use --). Also the attw script value has a leading space. Suggest changing the script to attw -P (no leading space) and updating callers to either npm run attw (if -P stays in the script) or npm run attw -- -P (if you want to pass args).
|
|
||
| - name: "Are the types wrong?" | ||
| run: yarn attw -P | ||
| run: npm run attw -P |
There was a problem hiding this comment.
CI runs npm run attw -P, but -P won’t be forwarded to the script without -- (and the attw script already hardcodes -P). Also @arethetypeswrong/cli in the updated lockfile declares engines.node >= 20, while this workflow pins Node 18, so the attw step is likely to fail at runtime. Consider updating the workflow matrix to Node 20+ and invoking the script as npm run attw (or npm run attw -- -P if you remove -P from the script).
| useEffect(() => { | ||
| const actualTarget = getTarget(target); | ||
|
|
||
| // @ts-ignore | ||
| actualTarget.addEventListener(type, onKeyEvent); | ||
| if (!actualTarget) return; | ||
|
|
||
| actualTarget.addEventListener(type, onKeyEvent as EventListener); | ||
|
|
||
| return () => { |
There was a problem hiding this comment.
When target is a RefObject, getTarget(target) may return null and the effect bails out. Because the effect deps only include the ref object identity, it won’t re-run if ref.current changes from null to an element later (common when the referenced element is conditionally rendered), so the listener may never attach. Consider reworking the effect to react to ref mount (e.g., attach on every render and track the last attached element, or attach to document and gate dispatch based on ref.current?.contains(e.target as Node)).
No description provided.