Skip to content
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

feat: new declareStore factory #15

Merged
merged 4 commits into from
Feb 1, 2023

Conversation

chartyom
Copy link
Contributor

@chartyom chartyom commented Jan 28, 2023

I prepared a new syntax for declare store. What do you think?
before

type State = {
  id: string;
};
const USER_STATE = {  
  id: string 
};
const USER_STATE_UPDATES = declareStateUpdates<State>()({
  setId: (id: number) => (state) => ({
    ...state,
    id: id,
  }),
});

 type UserStore = StoreWithUpdates<
  State,
  typeof USER_STATE_UPDATES
>;

const createUserStore = declareStoreWithUpdates(
  USER_STATE,
  USER_STATE_UPDATES,
  { name: 'UserStore' },
);

const userStore = createUserStore();
// OR
const userStore = createUserStore({id: 1});

userStore.updates.setId('2')

after

type State = {
  id: string;
};
const initialState: State = {
  id: '',
};
const createUserStore = declareStore({
  initialState,
  updates: {
    setId: (id: string) => (state) => ({
        ...state,
        id: id,
    }),
  },
});

type UserStore = ReturnType<typeof createUserStore>;

const userStore = createUserStore();
// OR
const userStore = createUserStore({ id: '1' });

userStore.updates.setId('2')

@coveralls
Copy link

coveralls commented Jan 28, 2023

Pull Request Test Coverage Report for Build 4065603664

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 3989539333: 0.0%
Covered Lines: 362
Relevant Lines: 362

💛 - Coveralls

@mnasyrov
Copy link
Owner

mnasyrov commented Jan 29, 2023

@chartyom First of all, thank you for the contribution!

Please consider the following points, because the proposed changes don't align with the library:

  • Please remove Deep* types for inferring a type for a state. Usage of a readonly, immutable state is not reposibility of the library. State type should be passed as it's done by existed createStore<State>().
  • Please reuse existed store an state types as much as possible. For example there is no need for the separate type StoreResult.
  • Related to the previous point, there is no need for reset() method of the store. Because an application logic is responsible for resetting a store, not the library.
  • The same is for getInitialState() - only application logic can decide for sure what should be considered as "initial state". Thus an initial state should not be an attribute of a store instance.
  • The proposed declareStore() shoud take only a value as an initial state, please don't provide a factory.

I'd like to propose you to remake existed declareStoreWithUpdates() as declareStore() with only changes:

  • Providing parameters as an object:
    declareState(params: {
      initialState: State,
      updates: Updates,
      options?: StoreOptions<State>,
    });
  • Reuse your magic for inferring state mutations (StateUpdates)
  • Attach a prodided updates to a resulting factory for a store, as you proposed. However, this property should be included to the common type for the factory, which will be returned by declaredStore() and declareStoreWithUpdates().


const localStore = createLocalStore(
{ userId: 1 },
{ comparator: (prevState, nextState) => prevState === nextState },
Copy link
Owner

Choose a reason for hiding this comment

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

It seems the custom comparator should be different from the base comparator to test properly that the custom on is used by the store.

(
initialState?: State | StateMutation<State> | null,
options?: StoreOptions<State>,
): Store<State> & StoreWithUpdates<State, CaseUpdates>;
Copy link
Owner

Choose a reason for hiding this comment

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

StoreWithUpdates<State, CaseUpdates> is enough here.

Suggested change
): Store<State> & StoreWithUpdates<State, CaseUpdates>;
): StoreWithUpdates<State, CaseUpdates>;

StoreWithUpdates,
} from './store';

interface StoreOptions<State> {
Copy link
Owner

Choose a reason for hiding this comment

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

Please use the original StoreOptions from store.ts to allow changing name for a specific store instance.

const { initialState, updates, name, comparator } = props;

const _state =
typeof initialState === 'function' ? initialState() : initialState;
Copy link
Owner

Choose a reason for hiding this comment

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

initialState() factory should be called only during creation of an actual store instance

Comment on lines 133 to 142
const _store = createStore<State>(_initialState, {
comparator: options?.comparator ?? comparator,
name,
onDestroy: options?.onDestroy,
});

return {
..._store,
updates: createStoreUpdates(_store.update, updates),
};
Copy link
Owner

Choose a reason for hiding this comment

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

Please reuse a code from declareStoreWithUpdates():

return withStoreUpdates<State, Updates>(
      createStore<State>(state, { ...baseOptions, ...options }),
      updates,
    );

onDestroy?: () => void;
}

interface StoreProps<
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
interface StoreProps<
type DeclareStoreOptions<...> = Readonly<{ ... }>

Comment on lines 24 to 30
name?: Name;

initialState: InitialState;

updates: Updates;
/** A comparator for detecting changes between old and new states */
comparator?: (prevState: State, nextState: State) => boolean;
Copy link
Owner

Choose a reason for hiding this comment

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

Consider to align with options of declareStoreWithUpdates(). I suggest:

  initialState: InitialState,
  updates: Updates,
  options?: StoreOptions<State>,

@mnasyrov
Copy link
Owner

mnasyrov commented Feb 1, 2023

Thank you for the PR. I consider to use declareStore() as the main method for store declaration and mark declareStoreWithUpdates() as deprecated.

@mnasyrov mnasyrov merged commit 824f156 into mnasyrov:main Feb 1, 2023
mnasyrov added a commit that referenced this pull request Feb 1, 2023
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.

3 participants