Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Question] Use of MobX toJS function #101

Open
tafty opened this issue Jan 18, 2023 · 1 comment
Open

[Question] Use of MobX toJS function #101

tafty opened this issue Jan 18, 2023 · 1 comment

Comments

@tafty
Copy link

tafty commented Jan 18, 2023

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
Copy link
Contributor

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))
  }
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants