Skip to content

Convert to TypeScript #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 31, 2025
Merged

Convert to TypeScript #31

merged 1 commit into from
May 31, 2025

Conversation

erikras
Copy link
Member

@erikras erikras commented May 31, 2025

Goodbye Flow! 👋

@erikras erikras requested a review from Copilot May 31, 2025 10:22
@erikras erikras merged commit 559342c into master May 31, 2025
3 checks passed
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Migrates the codebase from Flow to TypeScript, updating source files, types, and build/lint tooling.

  • Remove all Flow artifacts and introduce .ts/.tsx sources with corresponding type definitions
  • Update Rollup, Babel, ESLint, and package scripts to support TypeScript builds
  • Refresh dependencies, add CI workflows for linting, formatting, testing, and lock maintenance

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/warning.ts New TypeScript-based warning utility
src/warning.js Removed old Flow/JS warning implementation
src/types.ts New TypeScript interfaces for validation field props
src/types.js.flow Removed old Flow type definitions
src/index.ts New TypeScript entrypoint and exports
src/index.js.flow Removed old Flow entrypoint declaration
src/index.js Removed old JS export
src/Html5ValidationField.tsx Main component converted to TypeScript/TSX
src/Html5ValidationField.js Removed old JS component implementation
rollup.config.js Switched to TypeScript plugin, removed Flow plugin
package.json Added types, updated scripts, dependencies, peers
package-scripts.js Updated copyTypes to handle .d.ts files
eslint.config.js Added new ESLint config for TypeScript files
.eslintrc Updated React/TS lint rules and plugins
.babelrc.js Switched Babel presets to TypeScript, removed Flow
.github/workflows/lock.yml New workflow to lock stale issues/PRs
.github/workflows/ci.yml Updated CI to run lint, prettier, tests, coverage
Comments suppressed due to low confidence (4)

src/warning.ts:1

  • This new warning utility isn't covered by tests yet. Consider adding unit tests to validate that warnings are logged or suppressed correctly based on condition and environment.
export default function warning(condition: boolean, message: string): void {

src/types.ts:19

  • You reference React.Ref but React is not imported in this file. Add import * as React from 'react' (or import type React from 'react') to ensure React.Ref is defined.
  innerRef?: React.Ref<Html5ValidationField>

.eslintrc:8

  • [nitpick] The repository has both .eslintrc and eslint.config.js. ESLint may load both, causing conflicting rules. Consider consolidating into a single configuration format.
{

package.json:113

  • [nitpick] The bundlesize entries use inconsistent unit formatting (no space and uppercase KB). For consistency and clarity, format sizes uniformly (e.g., "1.6 kB").
      "maxSize": "1.6KB"

Comment on lines +3 to +8
if (typeof console !== 'undefined') {
console.error(`Warning: ${message}`)
}
try {
throw new Error(`Warning: ${message}`)
} catch (e) { }
Copy link
Preview

Copilot AI May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Flow version suppressed warnings in production via process.env.NODE_ENV. The new TS implementation always logs errors, changing behavior. Wrap the warning logic in an environment check to skip logging in production.

Suggested change
if (typeof console !== 'undefined') {
console.error(`Warning: ${message}`)
}
try {
throw new Error(`Warning: ${message}`)
} catch (e) { }
if (process.env.NODE_ENV !== 'production') {
if (typeof console !== 'undefined') {
console.error(`Warning: ${message}`)
}
try {
throw new Error(`Warning: ${message}`)
} catch (e) { }
}

Copilot uses AI. Check for mistakes.

Comment on lines +17 to +30
export interface Html5ValidationFieldProps extends FieldProps {
validate?: FieldValidator<unknown>
innerRef?: React.Ref<Html5ValidationField>
// All message keys
badInput?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
patternMismatch?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
rangeOverflow?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
rangeUnderflow?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
stepMismatch?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
tooLong?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
tooShort?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
typeMismatch?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
valueMissing?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
}
Copy link
Preview

Copilot AI May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The props interface re-declares all message keys already defined in Messages. To reduce duplication and simplify maintenance, extend Messages rather than re-listing those fields.

Suggested change
export interface Html5ValidationFieldProps extends FieldProps {
validate?: FieldValidator<unknown>
innerRef?: React.Ref<Html5ValidationField>
// All message keys
badInput?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
patternMismatch?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
rangeOverflow?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
rangeUnderflow?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
stepMismatch?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
tooLong?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
tooShort?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
typeMismatch?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
valueMissing?: string | ((value?: unknown, props?: Record<string, unknown>) => string)
}
export interface Html5ValidationFieldProps extends FieldProps, Messages {
validate?: FieldValidator<unknown>
innerRef?: React.Ref<Html5ValidationField>
}

Copilot uses AI. Check for mistakes.

import commonjs from 'rollup-plugin-commonjs'
import { uglify } from 'rollup-plugin-uglify'
import uglify from 'rollup-plugin-uglify'
Copy link
Preview

Copilot AI May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The uglify plugin is imported but never added to the plugins array, so the minified build won't be produced. Include umd && uglify() (or similar) in the appropriate build configuration.

Copilot uses AI. Check for mistakes.

@erikras erikras deleted the ts branch May 31, 2025 10:25
erikras added a commit that referenced this pull request May 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant