Apply Partial Update to Virtual Stores #100

jdolle · 2017-09-18T18:03:15Z

Rather than replacing the entire state on content scripts every time a change occurs, this change will create patch messages, containing only the updated fields. This has a number of benefits.

Brings behavior in line with standard redux store (expected behavior).
More efficient because less data is being transferred over the messaging pipeline.

Resolves #98

Patch content script state instead of always replacing

vhmth · 2017-09-18T22:57:42Z

src/wrap-store/wrapStore.js

-      });
+    let prevState = store.getState();
+
+    const patchState = () => {


This is where I was getting things wrong in my head. I like how you're doing the shallow diffing on the background page so the content script doesn't have to check equality at all (since all things serialized down will be new). Good shit. I need to make sure to grab a cup of ☕️ before talking in GH issues. :-P

vhmth · 2017-09-18T22:57:56Z

src/wrap-store/wrapStore.js

+          type: PATCH_STATE_TYPE,
+          payload: diff,
+        });
+      }
    };

    // Send new state down connected port on every redux store state change


Can we change this comment to read // Send patched state down...?

vhmth · 2017-09-18T22:58:08Z

test/shallowDiff.js

@@ -0,0 +1,39 @@
+// import should from 'should';


vhmth · 2017-09-18T22:59:30Z

src/wrap-store/shallowDiff.js

@@ -0,0 +1,36 @@
+const STATUS_UPDATED = 'updated';


Love this direction. Let's put these in a shared src/constants file and reference them here and in Store.js above.

vhmth · 2017-09-18T22:59:48Z

src/store/Store.js

+    const state = Object.assign({}, this.state);
+
+    difference.forEach(({change, key, value}) => {
+      if (change === 'updated') {


Love this. I like what you did before with the switch/case ladder - mind doing that here?

vhmth · 2017-09-18T23:01:49Z

src/wrap-store/wrapStore.js

-    // send initial state
-    sendState();
+    // Send store's initial state through port
+    port.postMessage({


Nice. Thanks for the cleanup by removing sendState!

vhmth · 2017-09-18T23:03:27Z

test/shallowDiff.js

+      {
+        key: 'a',
+        value: 2,
+        change: 'updated',


Loving these tests. When you extract out that constants file, could you reference the constants here as well?

jdolle · 2017-09-19T00:37:16Z

@vhmth Great feedback! I'm very glad you like the change. Please let me know if there is anything else I can do.

tshaddix · 2017-09-23T03:29:08Z

@jdolle @vhmth Hey guys - sorry I've been so out of the game. Had a family emergency I've been dealing with the last couple weeks.

@jdolle - really excited to look through this and see what you're proposing. I'm planning on diving in tomorrow morning. Thanks again for thinking through this!

vhmth · 2017-09-25T04:50:15Z

@tshaddix after you leave your feedback, I'll integrate into the Loom chrome extension repo locally to test out and see if there are any hiccups. I would say this is probably good to merge before then if we can get a more trivial example verified working.

tshaddix · 2017-09-26T02:30:22Z

@vhmth @jdolle Looking through this, this looks good to me. I do have some clarifying questions on the "root" problem we are trying to solve. Consider it devil's advocate and due diligence:

More efficient because less data is being transferred over the messaging pipeline. We have had this claim numerous times before. From a purely logical standpoint, you can say "sure that makes sense". That said, every time we have tried to quantify the impact it turns out to be negligible. Are we sure that sending state messages across the channel every time is more costly than a shallow diff every time? Not sure...

I would feel really confident in this change if we had some quantified proof of the performance issue. That said, I really don't see this change as having an earth shattering impact on perf either. So then we go back to "this brings us closer to Redux" which is true. I'm happy to move forward with testing it in Loom's extension, @vhmth . We won't be able to do that at GG right now, I'm afraid, or I would offer.

jdolle · 2017-09-26T03:47:27Z

@tshaddix I am new to browser extensions and haven't done any performance tests, so I can't quantify the gains.

It seems logical that copying less data over the messaging API would improve performance, but I think the major performance gain will be because of the simplification to prop comparisons. Overriding the entire state every time causes components, that might not otherwise, to be flagged as dirty and therefore re-render. React.PureComponent does === comparisons to avoid the performance hit from deep comparisons. By replacing state you replace the reference and therefore === will be false. It is true that you could override shouldComponentUpdate to compare someValue.id or whatever unique identifier there exists, but this is an extra step and adds complexity.

tshaddix · 2017-09-26T04:00:14Z

@jdolle Ah - great point. Don't know how we missed that in #10 and #12. Good call.

vhmth

@jdolle did a round of sanity checking and things look good to go on my end. I would wait up for @tshaddix to also give a thumbs up on his end.

tshaddix · 2017-10-08T17:13:07Z

@vhmth Alrighty - let's move this forward!

tshaddix · 2017-10-08T17:18:01Z

@vhmth I don't believe this warrants a major release semver-wise - thoughts? This seems to land under a minor release unless we consider the port-messages to be part of our package's public "API"

vhmth · 2017-10-08T17:19:49Z

🎉🎉🎉🎉 great stuff @jdolle!

@tshaddix roger roger - seems fit for a minor release.

jdolle added 2 commits September 18, 2017 10:49

Patch content script state instead of always replacing

ec65dce

Merge pull request #1 from Acornsgrow/feature/partial-update

04dab2b

Patch content script state instead of always replacing

vhmth reviewed Sep 18, 2017

View reviewed changes

test/shallowDiff.js Outdated

@@ -0,0 +1,39 @@

// import should from 'should';

Copy link

Collaborator

vhmth Sep 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nix

vhmth reviewed Sep 18, 2017

View reviewed changes

jdolle added 2 commits September 18, 2017 17:23

Move shallow diff states 'updated' and 'removed' into constants

da2e7c4

Use switch instead of if else

ddc82a6

tshaddix mentioned this pull request Oct 3, 2017

Add TypeScript type definition #102

Merged

vhmth approved these changes Oct 4, 2017

View reviewed changes

vhmth mentioned this pull request Oct 4, 2017

[Question] Is it possible to map only a part of the background redux store to the proxy store? #103

Closed

tshaddix merged commit 3e84d7f into tshaddix:master Oct 8, 2017

tshaddix mentioned this pull request Oct 8, 2017

Only send changes across state message channels #12

Closed

This was referenced Oct 13, 2017

Is immutable.js compatible with this library? #35

Closed

Updates to deep state not triggering connected component update #109

Closed

ericnkatz mentioned this pull request Feb 6, 2018

Issue with component rendering after update #122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply Partial Update to Virtual Stores #100

Apply Partial Update to Virtual Stores #100

jdolle commented Sep 18, 2017

vhmth Sep 18, 2017

vhmth Sep 18, 2017

vhmth Sep 18, 2017

vhmth Sep 18, 2017

vhmth Sep 18, 2017

vhmth Sep 18, 2017

vhmth Sep 18, 2017

jdolle commented Sep 19, 2017

tshaddix commented Sep 23, 2017

vhmth commented Sep 25, 2017

tshaddix commented Sep 26, 2017

jdolle commented Sep 26, 2017 •

edited

tshaddix commented Sep 26, 2017

vhmth left a comment

tshaddix commented Oct 8, 2017

tshaddix commented Oct 8, 2017

vhmth commented Oct 8, 2017

Apply Partial Update to Virtual Stores #100

Apply Partial Update to Virtual Stores #100

Conversation

jdolle commented Sep 18, 2017

vhmth Sep 18, 2017

Choose a reason for hiding this comment

vhmth Sep 18, 2017

Choose a reason for hiding this comment

vhmth Sep 18, 2017

Choose a reason for hiding this comment

vhmth Sep 18, 2017

Choose a reason for hiding this comment

vhmth Sep 18, 2017

Choose a reason for hiding this comment

vhmth Sep 18, 2017

Choose a reason for hiding this comment

vhmth Sep 18, 2017

Choose a reason for hiding this comment

jdolle commented Sep 19, 2017

tshaddix commented Sep 23, 2017

vhmth commented Sep 25, 2017

tshaddix commented Sep 26, 2017

jdolle commented Sep 26, 2017 • edited

tshaddix commented Sep 26, 2017

vhmth left a comment

Choose a reason for hiding this comment

tshaddix commented Oct 8, 2017

tshaddix commented Oct 8, 2017

vhmth commented Oct 8, 2017

jdolle commented Sep 26, 2017 •

edited