TypeScript: Store values should probably all infer as optional

[cboon-jeff]

Zedux Version

1.2.0

Description

If I have a Zedux store that has an object structure and I either pass in a type for the store as a generic or Zedux infers it, there is inadequate type-safety to prevent values getting unset. This happens because setStateDeep needs to be able to have any key be optional. So I can currently use this to set what should be a required field as undefined: store.setStateDeep({someRequiredField: undefined});. This will actually set the value to be undefined.

So I would assume that either all values for the store should be possibly undefined, or setStateDeep should ignore undefined values when merging. Below is a contrived example, in our case it was happening because we were getting one value from somewhere else and using it in setStateDeep.

Reproduction

import type { AtomStateType } from '@zedux/react';
import { atom, injectStore } from '@zedux/react';

type ExampleAtomState = {
  someArray: Array<number>;
  saveStatus: 'idle' | 'saving' | 'success' | 'error';
};

const defaultExampleAtomState: ExampleAtomState = {
  someArray: [1],
  saveStatus: 'idle',
};

const errorsIfUndefined = (v: string) => v;

export const exampleAtom = atom('example', () => {
  const store = injectStore<ExampleAtomState>(defaultExampleAtomState);
  console.log('store.getState()', store.getState());
  store.setStateDeep({ saveStatus: undefined });
  console.log('store.getState()', store.getState());
  const status = store.getState().saveStatus;
  console.log(errorsIfUndefined(status));
  return store;
});

So above I would either expect an error in TS on this line: console.log(errorsIfUndefined(status)); as I would have expected the inferred type of the store to essentially be a Partial<>, or that store.setStateDeep({ saveStatus: undefined }); is basically a no-op. I was thinking the former, but it does have the downside of always having to do extra checks that the value exists even if you aren't using setStateDeep anywhere. Could be better DX for Typescript users to prevent setting undefined in setStateDeep.

[bowheart]

@cboon-jeff thanks for reporting! I hadn't considered this before, I'll have to see why this is never causing problems for us. I agree that this behavior is not sufficiently type-safe.

I think any solution here would be a breaking change. I can tell already that making all state fields undefinable would be too disruptive a change. I'm in favor of making all undefined values passed to setStateDeep a no-op.

This would be a breaking change in this scenario:

const store = injectStore<{ foo?: number }>({ foo: 1 });
expect(store.getState().foo).toBe(1)
store.setStateDeep({ foo: undefined })
expect(store.getState().foo).toBeUndefined() // passes currently, would fail after this change

Changing the behavior of setStateDeep shouldn't be too disruptive for existing users. setStateDeep only exists for convenience and already has limited functionality (namely it can't delete fields). Since it is a breaking change, it will have to wait for Zedux v2, which we're starting to roadmap out right now, no target date yet.

Workarounds

I'm fine recommending that you use setState for now. It's less convenient, but should give an appropriate TS error when trying to set a non-undefinable field to undefined.

Alternatively, if you want improved store methods now and don't mind maintaining some code for it, here's an undocumented pattern that we've started using in several places:

Extend the Store class and create your own injector that injects instances of it. Here's an example that overrides the base Store class's setStateDeep method and uses lodash's omitBy to remove undefined keys.

class CustomStore<State> extends Store<State> {
  setStateDeep(settable: Settable<RecursivePartial<State>, State>, meta?: any) {
    const newState =
      typeof settable === 'function'
        ? (settable as (state: State) => RecursivePartial<State>)(this.getState())
        : settable;

    const filteredState =
      newState && typeof newState === 'object'
        ? (_.omitBy(newState, _.isUndefined) as RecursivePartial<State>)
        : newState;

    return super.setStateDeep(filteredState, meta);
  }
}

export const injectCustomStore = <State>(initialState: State, subscribe = true) => {
  const store = injectMemo(() => new CustomStore<State>(null, initialState), []);
  const self = injectSelf();

  injectEffect(
    () => {
      if (!subscribe) return;

      const subscription = store.subscribe(() => self.invalidate());

      return () => subscription.unsubscribe();
    },
    [subscribe],
    { synchronous: true }
  );

  return store;
};

I will officially document this pattern soon. No changes are planned for this, though note that it has a known TS issue when the store's state is an array. I'll release the fix for that before documenting this pattern.

[cboon-jeff]

Thanks for the fast and detailed response. This all sounds great, and thanks for giving us multiple solutions whilst we wait on a v2 implementation. Really appreciate it.