quarrant / mobx-persist-store

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

[Question] Use of MobX toJS function #101

Open tafty opened 1 year ago

tafty commented 1 year ago

In our project, we're seeing performance degradation that we've tracked down to the usage of the MobX toJS function in PersistStore.ts on ine 166. Swapping toJS(propertyData) out for the "classic" JSON.parse(JSON.stringify(properyData)) improves performance massively and the property's data appears to be exactly the same.

I'm wondering what makes the use of MobX toJS required here? Is it simply to dereference the propertyData? Or is it to account for implementations that may well be more complicated than our own? I'm not proposing changes, I'm simply trying to assess whether I can patch the package for now (we intend moving away from MobX later in the year).

I'm aware that there is now the option of using a SerializableProperty but if we implement individual property serialization, we will have to migrate existing stored data.

codeBelt commented 1 year ago

Thanks for your feedback. I don't know why we use toJS but I think changing to JSON.parse(JSON.stringify(properyData)) should be fine. 🤷

What performance degradation did you see? Could you provide an example of the data you are using in your app in a codesandbox instance?

On a side note, I am curious why you are moving away from MobX and what will you be moving to?


@quarrant Do you have time to look into this? I think we can clean up the code a little:

this.properties.forEach((property) => {
  const isComputedProperty = isComputedProp(target, property.key);
  const isActionProperty = isAction(target[property.key]);

  computedPersistWarningIf(isComputedProperty, String(property.key));
  actionPersistWarningIf(isActionProperty, String(property.key));

  if (!isComputedProperty && !isActionProperty) {
-    let propertyData = property.serialize(target[property.key]);
-
-    if (propertyData instanceof ObservableMap) {
-      const mapArray: any = [];
-      propertyData.forEach((v, k) => {
-        mapArray.push([k, toJS(v)]);
-      });
-      propertyData = mapArray;
-    }
-
-    propertiesToWatch[property.key] = toJS(propertyData);

+    const propertyData = property.serialize(target[property.key]);
+
+    propertiesToWatch[property.key] = JSON.parse(JSON.stringify(properyData))
  }
});