quarrant / mobx-persist-store

Persist and rehydrate observable properties in mobx store.
268 stars 14 forks source link

Type issues when using multiple serializable properties #97

Open Tartiflettus opened 1 year ago

Tartiflettus commented 1 year ago

Hello, first of all, great work on this package, the usage is intuitive, it's customizable, and it's not intrusive: one can use it on mobx stores basically as-is.

When using the package, I stumbled upon a possible issue when writing serializable properties. When using a single serializable property, everything works as intended, but when using 2 or more, the type checker complains about the arguments of the serializers.

Here is an example:

class Store {
  nb = 0;
  str = "0";

  constructor() {
    makeAutoObservable(this);
    makePersistable(this, {
      name: "Store",
      storage: window.localStorage,
      properties: [
        {
          key: "nb",
          serialize(v) { // v is deduced as this["nb" | "str"]
            return `${v}`;
          }
          deserialize(v) { // return type is deduced this["nb" | "str"]
            return parseFloat(v);
          }
        },
        {
          key: "str",
          serialize(v) { // same here for v
            return [v] as const;
          }
          deserialize(v) { // same here for return type
            return v[0];
          }
        }
      ]
    });
  }
}

After following the type declarations, it seems like the issue is that SerializableProperty is generic with 2 arguments: T (type of store), and P (key of store). So once in the properties array, these types are deduced in my example by Store and "nb" | "str".

While the enum deduction is correct when properties is defined as an array of the keys, it leads to this weird behaviour when using serializable properties, where all serializers must handle all used types.

From my perspective, it seems like the required behaviour would be to have each serializable property use it's own P, but after searching for a bit, I don't think that typescript allows this kind of thing :/. For now, I "solved" this by using a separate function that is correctly typed for each use, but degenerates its output type to use any (codesandbox).

What do you think about it, was this indeed not intended, or am I misusing the package?

In all cases have a nice day

quarrant commented 1 year ago

Great job! Thank for your issue. I’ll try to fix it on the weekend

quarrant commented 1 year ago

@viicky fixed in 1.1.3

Tartiflettus commented 1 year ago

Thank you!

dansimau commented 1 year ago

I am still having the same issue in 1.1.3.

Here is a very simple example:

import { makeAutoObservable } from "mobx";
import { makePersistable } from "mobx-persist-store";

type Cookie = {
  name: string;
  isBaking: boolean;
};

export class ExampleStore {
  cookies = new Map<string, Cookie>();
  ovenSettings = {
    temperature: 200,
  };

  constructor() {
    makeAutoObservable(this, {}, {autoBind: true});
    makePersistable(this, {
      name: "CookieStore",
      properties: [
        {
          key: "ovenSettings",
          serialize: v => v,
          deserialize: v => v,
        },
        {
          key: "cookies",
          serialize: v => v,
          deserialize: v => v,
        },
      ],
    });
  }
}

Result:

index.ts:26:11 - error TS2322: Type '"cookies"' is not assignable to type '"ovenSettings"'.
index.ts:27:11 - error TS2322: Type '(v: this["cookies"]) => this["cookies"]' is not assignable to type '(value: this["ovenSettings"]) => any'.
dansimau commented 1 year ago

Okay I discovered it compiles fine in TS 4.4.4 (on CodeSandbox) but not in the latest version (4.9.4). It seems like the above error starts from TS 4.6 onwards.

quarrant commented 1 year ago

I’ll check it

Redmega commented 1 year ago

I get a similar issue when trying to mix string and serializable properties:

  constructor(store: RootStore) {
    this._store = store;
    makeAutoObservable(this, { _store: false }, { autoBind: true });
    makePersistable(this, {
      name: "GameStore",
      storage,
      properties: [
        "combat",
        "_lastUpdated",
        {
          key: "workees",
          serialize(value) {
            return value;
          },
          deserialize(_workees: Workee[]) {
            return _workees.map(workee => new Workee(workee));
          },
        },
      ],
    });
    log.info("initialized");
  }

This gives me an error on "combat" and "_lastUpdated"

Type '"combat"' is not assignable to type '"workees" | { key: "workees"; serialize: (value: this["workees"]) => any; deserialize: (value: any) => this["workees"]; }'

Type '"_lastUpdated"' is not assignable to type '"workees" | { key: "workees"; serialize: (value: this["workees"]) => any; deserialize: (value: any) => this["workees"]; }'
spencercap commented 1 year ago

Yes, this is still happening w "mobx-persist-store": "^1.1.3", and "typescript": "^5.1.3",

Any advice?

ArturBaybulatov commented 9 months ago

I had a similar problem with mixed-up keys:

image

Currently I've worked around the problem using the explicit satisfies operator:

image

The type has been imported like this:

import { SerializableProperty } from 'mobx-persist-store/lib/esm2017/serializableProperty';
Maxoos commented 4 months ago

any news on this?