Add react implementation of state management#124
Add react implementation of state management#124andrew-polk wants to merge 1 commit intoui-controllerfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a React adapter for the @ethnolib/state-management-core library, enabling React applications to use the framework-agnostic state management system. The implementation follows the pattern established by the existing Svelte adapter, providing a useField hook that binds Field objects to React state.
Key Changes
- Created new
@ethnolib/state-management-reactpackage with theuseFieldhook - Added build configuration (Vite, TypeScript, Vitest) following the existing pattern
- Included comprehensive README documentation explaining usage patterns
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| package-lock.json | Added dependencies for the new React state management package and updated Svelte package references |
| components/state-management/state-management-react/package.json | Package manifest defining the React adapter with build scripts and dev dependencies |
| components/state-management/state-management-react/index.ts | Main entry point exporting the useField hook |
| components/state-management/state-management-react/src/use-field.ts | Core implementation of the React hook for binding Field objects to React state |
| components/state-management/state-management-react/vite.config.ts | Vite build configuration for library mode |
| components/state-management/state-management-react/vitest.config.ts | Vitest test configuration |
| components/state-management/state-management-react/tsconfig.json | Base TypeScript configuration with React JSX support |
| components/state-management/state-management-react/tsconfig.lib.json | Library-specific TypeScript configuration for build output |
| components/state-management/state-management-react/README.md | Documentation explaining the state management system and usage patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@vitejs/plugin-react-swc": "^3.8.0", | ||
| "tsx": "^4.19.2", | ||
| "typescript": "^5.2.2" | ||
| }, |
There was a problem hiding this comment.
Missing peerDependencies for "react" (and optionally "react-dom"). Since this package is a React adapter that imports from "react", it should declare React as a peer dependency to ensure consumers have it installed. Consider adding:
"peerDependencies": {
"react": "^17.0.0 || ^18.0.0"
}| }, | |
| }, | |
| "peerDependencies": { | |
| "react": "^17.0.0 || ^18.0.0" | |
| }, |
| _setFieldValue(value); | ||
| } | ||
|
|
||
| field.updateUI = (value) => _setFieldValue(value); |
There was a problem hiding this comment.
The field.updateUI assignment on every render can cause issues. If the component re-renders (e.g., due to parent state changes), this assignment will overwrite the callback, potentially breaking synchronization. Consider using useEffect to set this up once:
useEffect(() => {
field.updateUI = (value) => _setFieldValue(value);
return () => {
field.updateUI = null; // cleanup
};
}, [field]);| export function useField<T>(field: Field<T>): [T, (value: T) => void] { | ||
| const [fieldValue, _setFieldValue] = useState(field.value); | ||
|
|
||
| function setFieldValue(value: T) { | ||
| field.requestUpdate(value); | ||
| _setFieldValue(value); | ||
| } | ||
|
|
||
| field.updateUI = (value) => _setFieldValue(value); | ||
|
|
||
| return [fieldValue, setFieldValue]; | ||
| } |
There was a problem hiding this comment.
The useField hook lacks test coverage. The sibling package state-management-svelte has tests (e.g., transform-view-model.test.ts) for similar functionality. Consider adding tests to verify:
- Initial state matches field value
- Updates through setFieldValue call field.requestUpdate
- Updates through field.updateUI update React state
- Cleanup of field.updateUI on unmount
| @@ -0,0 +1,108 @@ | |||
| # State Management Core | |||
|
|
|||
| This package provides a simple interface for working with multiple reactive framworks. | |||
There was a problem hiding this comment.
Typo: "framworks" should be "frameworks"
| This package provides a simple interface for working with multiple reactive framworks. | |
| This package provides a simple interface for working with multiple reactive frameworks. |
| const person = usePersonViewModel(); | ||
|
|
||
| // Replace `defineReactiveState` with your framework's mechanism | ||
| const reactiveName = defineReactiveSate(); |
There was a problem hiding this comment.
Typo: "defineReactiveSate" should be "defineReactiveState"
| const reactiveName = defineReactiveSate(); | |
| const reactiveName = defineReactiveState(); |
| export default defineConfig({ | ||
| root: __dirname, | ||
| cacheDir: | ||
| "../../../node_modules/.vite/components/state-management/state-management-core", |
There was a problem hiding this comment.
The cacheDir path references "state-management-core" but should reference "state-management-react" to match the actual package being built.
| "../../../node_modules/.vite/components/state-management/state-management-core", | |
| "../../../node_modules/.vite/components/state-management/state-management-react", |
| }, | ||
| lib: { | ||
| entry: "./index.ts", | ||
| name: "@ethnolib/find-language", |
There was a problem hiding this comment.
The library name is incorrectly set to "@ethnolib/find-language". It should be "@ethnolib/state-management-react" to match the package name.
| name: "@ethnolib/find-language", | |
| name: "@ethnolib/state-management-react", |
| "declarationMap": true | ||
| }, | ||
| "exclude": [ | ||
| "langtagProcessing.ts", |
There was a problem hiding this comment.
The exclude pattern references "langtagProcessing.ts" which doesn't exist in this package. This appears to be a leftover from a template and should be removed.
| "langtagProcessing.ts", |
1 participant
This change is