Convert to TypeScript #47
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the codebase from Flow/JavaScript to TypeScript, updates build tooling, and refreshes dependencies and CI/workflow configs.
- Rewrite
OnChange,OnBlur, andExternallyChangedcomponents and tests in TypeScript - Switch Rollup from Flow plugin to
rollup-plugin-typescript2and update Babel presets - Upgrade dependencies in
package.jsonand adjust CI/app scripts for linting and typechecking
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/OnChange.tsx | Converted listener component to TS/TSX with hooks |
| src/OnBlur.tsx | Migrated blur listener component to TypeScript |
| src/ExternallyChanged.tsx | Migrated external-change tracker component to TS/TSX |
| rollup.config.js | Swapped Flow plugin for TypeScript, updated input |
| package.json | Added "types", bumped dev dependencies, TS/Jest |
| .github/workflows/ci.yml | CI configuration added (lint, prettier, test, coverage) |
| // is designed to run on every render. It compares the current and previous | ||
| // values of `input.value` and triggers the `children` callback if they differ. | ||
| React.useLayoutEffect(() => { | ||
| if (!hasInitializedRef.current) { |
There was a problem hiding this comment.
Your tests expect the listener to be called on first render, but this branch skips calling children. You should invoke children(input.value, previousValueRef.current) during initial setup or update the tests to align with intended behavior.
| if (!hasInitializedRef.current) { | |
| if (!hasInitializedRef.current) { | |
| children(input.value, previousValueRef.current) |
| children(input.value, previousValueRef.current) | ||
| previousValueRef.current = input.value | ||
| } | ||
| }) |
There was a problem hiding this comment.
[nitpick] This useLayoutEffect intentionally omits its dependency array, but will trigger a lint warning. Consider adding // eslint-disable-next-line react-hooks/exhaustive-deps or specifying dependencies explicitly for clarity.
|
|
||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - name: Use Node.js ${{ matrix.node_version }} | ||
| uses: actions/setup-node@v2 | ||
| with: | ||
| node-version: "22" |
There was a problem hiding this comment.
Specifying Node.js version "22" may fail since it's not a published LTS. Either define a strategy.matrix.node_version with supported versions (e.g., 18, 20) or use an existing LTS like "18".
| steps: | |
| - uses: actions/checkout@v2 | |
| - name: Use Node.js ${{ matrix.node_version }} | |
| uses: actions/setup-node@v2 | |
| with: | |
| node-version: "22" | |
| strategy: | |
| matrix: | |
| node_version: [18, 20] | |
| steps: | |
| - uses: actions/checkout@v2 | |
| - name: Use Node.js ${{ matrix.node_version }} | |
| uses: actions/setup-node@v2 | |
| with: | |
| node-version: ${{ matrix.node_version }} |
No description provided.