quarrant / mobx-persist-store

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

makePersistable is not saving an ES6 Map to the local storage correctly. Possible solution in the comment. #41

Closed BornaGajic closed 3 years ago

BornaGajic commented 3 years ago

Hi! I'm using Typescript and ran into a problem where makePersistable wouldn't save a Map properly to local storage e.g. content would be empty - {}. With a little digging, I found that in the StorageAdapter.js content is being stringified without the use of the replacer function for JSON.strigify method which led to the problem.

Consider a possible working solution for JS:

image _nodemodules\mobx-persist-store\lib\StorageAdapter.js

Maybe add an option in makePersistable to pass in a replacer function for JSON stringify

codeBelt commented 3 years ago

Thanks for the feedback. I will have to look at this later but try setting stringify: false, to see if that fixes the issue for now.

https://github.com/quarrant/mobx-persist-store/tree/issue-36/stable-release#storageoptions--reactionoptions

Also here is the example I am working on for v1.0 of mobx-persist-store: https://codesandbox.io/s/mobx-persist-store-version-1-f852w?file=/src/stores/User.store.ts

BornaGajic commented 3 years ago

Thanks for the feedback. I will have to look at this later but try setting stringify: false, to see if that fixes the issue for now.

https://github.com/quarrant/mobx-persist-store/tree/issue-36/stable-release#storageoptions--reactionoptions

Also here is the example I am working on for v1.0 of mobx-persist-store: https://codesandbox.io/s/mobx-persist-store-version-1-f852w

setting stringify: false doesn't solve the problem.

codeBelt commented 3 years ago

For now you can create your own storage adapter to handle your situation:

makePersistable(this, {
  name: 'SampleStore',
  properties: ['someProperty'],
  storage: {
    setItem: () => {
      // Add a customizes version of window.localStorage.setItem to handle Map
      // return window.localStorage.setItem;
    },
    removeItem: window.localStorage.removeItem,
    getItem() {
      // Add a customizes version of window.localStorage.getItem if needed
      return window.localStorage.getItem;
    },
  },
  stringify: false,
});

I found this article https://2ality.com/2015/08/es6-map-json.html

codeBelt commented 3 years ago

@BornaGajic I am curious what you come up with for your custom storage adapter to solve this issue. If you could, create a codesandbox example and share here.

BornaGajic commented 3 years ago

@BornaGajic I am curious what you come up with for your custom storage adapter to solve this issue. If you could, create a codesandbox example and share here.

I solved my original issue (with the code I posted in the original issue text and with the storageOptions you provided), but another popped up.

Second issue is with the getItem method in storageOptions returning string. The problem is that I can't declare how I want to change the (domain) object before it ships out to the code. In other words, getItem returns string - when I refresh the page my domain object is parsed as it should be until it reaches what should be Map field which is parsed as JS Object. What I want is to let JSON.parse do it's work but your method (getItem) should let me modify that Object before it is shipped to the code. So I suggest a change to the Object as the return type.

Section 2.2 in the last link you provieded.

I'll post codesandbox that reproduces the problem later.

But, I may be wrong. I'll review it again later.

BornaGajic commented 3 years ago

https://codesandbox.io/embed/mobx-persist-store-map-bug-forked-bfk1q?fontsize=14&hidenavigation=1&theme=dark

Steps for reproduction:

codeBelt commented 3 years ago

You need to update your getItem to hand converting the data back into a Map object:

getItem: (key: string) => {
  let data: any = window.localStorage.getItem(key);

  data = JSON.parse(data as string);

  const strMap = new Map();

  for (const k of Object.keys(data.myMap)) {
    strMap.set(k, data.myMap[k]);
  }

  return {
    ...data,
    myMap: strMap,
  };
}

The code is ugly but hopefully you can see how to do it.

BornaGajic commented 3 years ago

Thanks @codeBelt I've managed to solve both issues.

Here's the final codesandbox: https://codesandbox.io/embed/mobx-persist-store-map-bug-forked-bfk1q?fontsize=14&hidenavigation=1&theme=dark

Edit: Updated code

Not the best implementation but I'll leave it for now like this. Currently, my code depends on modelMap which storages classNames and new instances of that class. I'm checking if there's a class with the same name as the key of the JSON object - if there is, assign all children to that class instance, if not then create a Map from it.