atom.io/no more calling families

jeremybanka commented 1 month ago

User description

🏷️ atom/selectorFamily do not return function type
🦋

PR Type

Enhancement, Tests, Documentation

Description

Updated various test files to use the findState function instead of direct function calls.
Removed deprecated RegularAtomFamilyTokenWithCall, MutableAtomFamilyTokenWithCall, WritableSelectorFamilyTokenWithCall, and ReadonlySelectorFamilyTokenWithCall types.
Updated atomFamily and selectorFamily function signatures to remove deprecated types.
Added a changeset note indicating that calling atomFamily or selectorFamily directly now results in a TypeScript error.

Changes walkthrough 📝

Relevant files

Enhancement

game-store.ts Update `letterIndex` to use `findState` function packages/atom.io/__tests__/experimental/realtime-ipc/game-store.ts Imported `findState` from `atom.io/ephemeral`. Updated `letterIndex` to use `findState` instead of direct function call.	+2/-1
atom.ts `Remove deprecated RegularAtomFamilyTokenWithCall and MutableAtomFamilyTokenWithCall types` packages/atom.io/src/atom.ts Removed deprecated `RegularAtomFamilyTokenWithCall` and `MutableAtomFamilyTokenWithCall` types. Updated `atomFamily` function signatures to remove deprecated types.	+3/-30
selector.ts `Remove deprecated WritableSelectorFamilyTokenWithCall and ReadonlySelectorFamilyTokenWithCall types` packages/atom.io/src/selector.ts Removed deprecated `WritableSelectorFamilyTokenWithCall` and `ReadonlySelectorFamilyTokenWithCall` types. Updated `selectorFamily` function signatures to remove deprecated types.	+5/-27

Tests

async-data.test.ts Refactor `AtomIO.setState` calls to use `findState` packages/atom.io/__tests__/private/async-data.test.ts Modified `AtomIO.setState` calls to use `findState` functions with separate key and value parameters.	+5/-5
grace.test.ts Use `findState` in `ftl` function packages/atom.io/__tests__/private/grace.test.ts Updated `ftl` function to use `findState` instead of direct function call.	+1/-1
dict.test.ts Refactor `setState` call to use `findState` packages/atom.io/__tests__/public/data/dict.test.ts Modified `setState` call to use `findState` with separate key and value parameters.	+1/-1

Documentation

lemon-islands-train.md

Document TypeScript error for direct atomFamily or selectorFamily 
calls

.changeset/lemon-islands-train.md

Added changeset note indicating that calling atomFamily or
selectorFamily directly now results in a TypeScript error.

+5/-0

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

vercel[bot] commented 1 month ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
atom-io-fyi	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 11, 2024 3:07am
wayfarer-quest	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 11, 2024 3:07am

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 0747525a83109ec30e0a854ae3318b34fd2256c9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | ------- | ----- | | atom.io | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review

Refactor Required
The replacement of direct function calls with `findState` should be validated to ensure that it behaves identically in all cases, especially in asynchronous or complex state scenarios. Major Refactoring
The removal of `WithCall` types from `atomFamily` and `selectorFamily` functions simplifies the API but requires thorough testing to ensure no functionality is lost or behaves unexpectedly. API Change
The API change in `selectorFamily` functions should be clearly documented, and migration steps should be provided for users who might be affected by the removal of `WithCall` types.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Performance	Use batch operations for setting multiple states ___ Use a batch operation for setting multiple states to potentially improve performance by minimizing re-renders or state updates. [packages/atom.io/__tests__/private/async-data.test.ts [139-145]](https://github.com/jeremybanka/wayforge/pull/2383/files#diff-b29932023d2ca6f7e4dcb801a6946c6d11465f155ae363650fad8998cb1eddd0R139-R145) ```diff -AtomIO.setState(findCountState, key, value) -AtomIO.setState(findLetterState, key, value) -AtomIO.setState(findStatusState, key, value) +AtomIO.batch(() => { + AtomIO.setState(findCountState, key, value) + AtomIO.setState(findLetterState, key, value) + AtomIO.setState(findStatusState, key, value) +}) ``` Suggestion importance[1-10]: 8 Why: Using batch operations for setting multiple states can significantly improve performance by minimizing re-renders or state updates. This is a valuable improvement.	8
Performance	Optimize the default value generation for better performance ___ Ensure that the default value generation for `letterIndex` is optimized for performance, especially if this operation is frequent or involves large data sets. [packages/atom.io/__tests__/experimental/realtime-ipc/game-store.ts [13]](https://github.com/jeremybanka/wayforge/pull/2383/files#diff-6177f840254c0f6bfeea6f352c1e95828e7ae19409c2fa4ac8d52aeff0db1180R13-R13) ```diff -default: Array.from({ length: 5 }).map((_, i) => findState(letterAtoms, i)), +default: [...Array(5)].map((_, i) => findState(letterAtoms, i)), ``` Suggestion importance[1-10]: 6 Why: The suggested change to use `[...Array(5)]` instead of `Array.from({ length: 5 })` is a minor optimization. While it may improve performance slightly, the impact is likely minimal in this context.	6
Possible issue	Add error handling for state retrieval ___ Consider checking the existence of the state before using `findState` to avoid potential runtime errors if the state does not exist. [packages/atom.io/__tests__/experimental/realtime-ipc/game-store.ts [173]](https://github.com/jeremybanka/wayforge/pull/2383/files#diff-6177f840254c0f6bfeea6f352c1e95828e7ae19409c2fa4ac8d52aeff0db1180R173-R173) ```diff -const WritableToken = findState(f, key) +const WritableToken = findState(f, key) ?? throw new Error('State not found'); ``` Suggestion importance[1-10]: 7 Why: Adding error handling for state retrieval is a good practice to avoid potential runtime errors. However, the suggested code uses a syntax that is not valid in TypeScript. A better approach would be to use an if-statement to check for the existence of the state.	7
Maintainability	Improve variable naming for clarity ___ Consider using a more descriptive variable name than `key` in the loop to enhance code readability and maintainability. [packages/atom.io/__tests__/private/async-data.test.ts [139]](https://github.com/jeremybanka/wayforge/pull/2383/files#diff-b29932023d2ca6f7e4dcb801a6946c6d11465f155ae363650fad8998cb1eddd0R139-R139) ```diff -for (const [key, value] of Object.entries(counts)) { - AtomIO.setState(findCountState, key, value) +for (const [countKey, countValue] of Object.entries(counts)) { + AtomIO.setState(findCountState, countKey, countValue) } ``` Suggestion importance[1-10]: 5 Why: Using more descriptive variable names enhances code readability and maintainability. While this is a good practice, it is a minor improvement.	5

coveralls commented 1 month ago

coverage: 91.522%. remained the same when pulling 0747525a83109ec30e0a854ae3318b34fd2256c9 on atom.io/no-more-calling-families into 663cdd4902d32c34e9022ed610551d14cd64b310 on main.

jeremybanka / wayforge