Convert to TypeScript #99
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR converts the Final Form Arrays project from Flow to TypeScript, updating source files, tests, and build configurations.
- Replaces Flow type annotations with TypeScript equivalents in all source files.
- Updates tests to include proper TypeScript type annotations and casts.
- Revises build configuration and package scripts to support TypeScript.
Reviewed Changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/push.ts | Converted to TypeScript with updated type annotations. |
| src/pop.ts | Converted to TypeScript with adjusted return types. |
| src/move.ts | Removed Flow types; updated function signature and typings. |
| src/insert.ts | Converted to TypeScript; updated inline type annotations. |
| src/concat.ts | Converted to TypeScript with proper parameter type definitions. |
| src/index.ts | Updated module imports/exports to work with TypeScript. |
| src/copyField.ts | Replaced Flow types with TypeScript, using “any” for compatibility. |
| rollup.config.mjs | Switched from Flow to TypeScript via rollup-plugin-typescript2. |
| package.json | Updated scripts and dependencies to incorporate TypeScript. |
| package-scripts.js | Revised build scripts to generate TypeScript declarations. |
| eslint.config.mjs | Added ESLint configuration updates for TypeScript files. |
| .babelrc | Replaced Flow preset with TypeScript preset. |
There was a problem hiding this comment.
Pull Request Overview
This pull request converts the library and its tests from Flow to TypeScript, removes Flow-specific tooling, and updates build/lint configurations to support TypeScript.
- Migrate source files and tests to TypeScript, replacing Flow types with TS types and introducing
createMockToolsin tests - Remove Flow plugins/presets and add
rollup-plugin-typescript2, updatetscscripts and dependencies - Update
src/index.tsexport API and remove redundant Flow type declarations
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pop.ts | Removed Flow import, added TS return type, adjusted signature |
| src/move.ts | Updated to TS syntax and types, removed Flow types |
| src/insert.ts | Migrated to TS, removed Flow types, updated comment |
| src/index.ts | Changed default export to mutators object and added named exports |
| rollup.config.mjs | Switched from Flow plugin to TypeScript plugin, updated input |
| package.json | Added TS build scripts, bumped deps for TS support |
| .github/workflows/ci.yml | Added CI workflow but references undefined matrix.node_version |
| src/copyField.ts | Replaced specific Flow type with any for fields |
| src/pop.test.ts | Tests cast state shape to any extensively |
Comments suppressed due to low confidence (2)
src/insert.ts:16
- [nitpick] The comment has a grammatical issue—update it for clarity, e.g.,
// now increment any higher indices.
// now we have increment any higher indexes
.github/workflows/ci.yml:12
- The step references
matrix.node_versionbut no matrix is defined; either define the matrix or hardcode the version to avoid CI misconfiguration.
- name: Use Node.js ${{ matrix.node_version }}
There was a problem hiding this comment.
Pull Request Overview
This pull request converts Final Form Arrays from Flow to TypeScript, updating type annotations, test utilities, and build configurations accordingly. Key changes include replacing Flow type comments and type exports with TypeScript syntax, updating tests to use proper type assertions and a new mock tools helper, and adjusting build scripts and configuration files for TypeScript support.
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pop.ts | Converted pop mutator from Flow to TypeScript |
| src/pop.test.ts | Updated pop tests with TypeScript typings and createMockTools |
| src/move.ts & src/move.test.ts | Converted move mutator and tests to TypeScript |
| src/insert.ts & src/insert.test.ts | Converted insert mutator and tests to TypeScript |
| src/index.ts | Updated index file to export TypeScript mutators and types |
| Other files (copyField, concat, etc.) | Converted remaining mutators and their tests to TypeScript, updated build configuration and dependency management accordingly |
Comments suppressed due to low confidence (1)
src/pop.test.ts:8
- [nitpick] Consider using more specific type definitions instead of frequent 'as any' assertions in test state initializations to improve type safety.
const state: MutableState<any> = { ... } as any
There was a problem hiding this comment.
Pull Request Overview
This pull request converts the project from Flow to TypeScript and updates all related tooling and tests accordingly. Key changes include removing Flow annotations and adding explicit TypeScript types across source and test files, updating build configurations (Rollup, Babel, package scripts), and reworking module exports for TypeScript.
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/*.ts | Converted mutators from Flow to TypeScript with updated type casts. |
| src/*.test.ts | Updated tests to use TypeScript types and createMockTools for consistency. |
| rollup.config.mjs & package.json | Modified build configurations and dependency updates for TypeScript. |
| Other files (copyField, index, etc.) | Updated typings and module exports to align with the TypeScript migration. |
| import remove from './remove' | ||
|
|
||
| const pop: Mutator<any> = ( | ||
| [name]: any[], | ||
| state: MutableState<any>, | ||
| tools: Tools<any> | ||
| ) => { | ||
| ): any => { |
There was a problem hiding this comment.
Consider refining the return type of the pop function to more precisely reflect the value returned by remove, rather than using 'any'.
Copilot uses AI. Check for mistakes.
| expect(result).toBeUndefined() | ||
| expect(changeValue).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('should call changeValue once', () => { | ||
| const changeValue = jest.fn() | ||
| const state = { fields: {} } | ||
| const result = move(['foo', 0, 2], state, { changeValue }) | ||
| const state: MutableState<any> = { fields: {} } as any |
There was a problem hiding this comment.
[nitpick] Where possible, define a properly typed mock state instead of relying on 'as any' casts to improve type safety and maintainability in tests.
| const state: MutableState<any> = { fields: {} } as any | |
| const state: MutableState<any> = { fields: {} } |
Copilot uses AI. Check for mistakes.
| function copyField( | ||
| oldFields: { [string]: InternalFieldState }, | ||
| oldFields: { [key: string]: InternalFieldState<any> }, | ||
| oldKey: string, | ||
| newFields: { [string]: InternalFieldState }, | ||
| newFields: { [key: string]: Partial<InternalFieldState<any>> }, |
There was a problem hiding this comment.
[nitpick] If feasible, consider replacing the generic 'any' with a more specific type parameter for InternalFieldState to further improve type safety and maintainability.
Copilot uses AI. Check for mistakes.
No description provided.