Foolproofing StateField

v6
1

I had naively implemented the update of my StateField as:

    update(foos: Foo[], tr: Transaction): Foo[] {
      for (const effect of tr.effects) {
        if (effect.is(pushFoo)) foos.push(effect.value);
        if (effect.is(popFoo)) foos.pop();
      }
      return foos;
    },

The mistake being that you shouldn’t mutate the given value but return a new one. This only had (poorly) worked because I had left compare⁠ unimplemented, which defaults to ===.

I think it would be nice if the signature:

update: (value: Value, transaction: Transaction) => Value;

would be changed to:

update: (value: Readonly<Value>, transaction: Transaction) => Readonly<Value>;

to prevent such stupid mistakes. Or alternatively the compare default implementation could be removed or codemirror could log an error if create returns an array or an object and compare isn’t implemented since in that case you’ll definitely want to implement it.

2

Also just occurred to me: StateFieldSpec could be made conditional to require compare for records and arrays:

type StateFieldSpec<Value> = Value extends Record<string, unknown> | unknown[]
  ? { compare: (a: Value, b: Value) => boolean }
  : { compare?: (a: Value, b: Value) => boolean };

// ok
const a: StateFieldSpec<number> = {};

// type error
const b: StateFieldSpec<{ name: string }> = {};

// type error
const c: StateFieldSpec<string[]> = {};
3

Unfortunately, TypeScript has no real way of annotating that a type should be immutable (you’ll still have the same issue with nested objects even if you use Readonly, and randomly adding it to method signatures feels messy when what you actually want to encode is a constraint on the type given for Value itself) So I think we’ll just have to continue to rely on documentation here, and hope people read it.

It’s often perfectly reasonable to rely on identity compare for such types. Since they are immutable, you’ll generally only replace them with a new value when something changed.

1 Like