Serialization hooks #126

ethanroday · 2018-01-12T22:56:54Z

Add new serializer and deserializer options for wrapStore and Store, which will serialize all outgoing messages and deserialize all incoming messages.
Update README

Note: When testing in an actual extension, I found it necessary to wrap all of the messaging functions (e.g. port.postMessage or chrome.runtime.onMessage.addListener) in anonymous functions, rather than passing the function references directly. If I didn't, Chrome gave me the following error:

Uncaught TypeError: Method called without a valid receiver (this). Did you forget to call .bind()?
  at getPrivateImpl (extensions::utils:119:16)
  at publicClassPrototype.(anonymous function) (extensions::utils:137:20)
...

I'm not sure what's causing this, but would welcome ideas. It feels unnecessary to have to wrap all of the calls.

…o Store and wrapStore

This reverts commit a3fd0c1.

vhmth · 2018-01-15T19:20:05Z

@ethanroday yeah by default our store functions aren't bound to the instance (we can get around this in the future by using Class properties). I will get to reviewing this PR in a bit today. Thank you so much for this beast. The documentation looks stellar, and I think this is a sane entrypoint into giving people flexibility without sacrificing the default, out-of-the-box redux behavior.

ethanroday · 2018-01-18T03:58:32Z

@vhmth got it. Glad you think it makes sense! It’s a little suboptimal in the sense that it exposes some internals to the user (patch message payloads are more than just slices of the store’s state), but I imagine in most cases that will be okay.

ethanroday · 2018-01-23T19:09:48Z

Hey @vhmth, have you had a chance to look this over?

ethanroday · 2018-02-09T16:11:54Z

@vhmth, @tshaddix have either of you had a chance to review?

tshaddix · 2018-02-14T20:40:40Z

@ethanroday Have this on my todos. Trying to get to this today!

tshaddix

@vhmth This looks good to me after going through it. Would love your thoughts as well.

The only change that is required is to fix whatever is breaking the build after the merge from master. The master merge just contained a simple addition from #134 to make the extension id null instead of an empty string.

tshaddix · 2018-03-22T23:07:52Z

README.md

+    {
+      id: 1,
+      text: 'Write a Chrome extension',
+      created: {}


Is this true? I swore it turned it into a string last time I checked... am I crazy?

Vanilla JSON.parse(JSON.stringify(new Date())) will give you a string, but Chrome's default serialization/deserialization for messaging gives you an empty object for some reason or another.

Got it - must have been confusing the two. Thanks

Wow this is really good to know. I could see us having a serializers dir with defaults for things like dates (not necessary for this PR ofc).

tshaddix · 2018-03-22T23:08:23Z

README.md

+
+wrapStore(store, {
+  portName: 'MY_APP',
+  serializer: payload => JSON.stringify(payload, dateReplacer),


I like that these were added as options rather than additional parameters to wrap store 👍

I like the names too (dateReplacer and dateReviver). Makes it clear what's going on

tshaddix · 2018-03-22T23:12:27Z

src/serialization.js

@@ -0,0 +1,91 @@
+export const noop = (payload) => payload;
+
+const transformPayload = (message, transformer = noop) => ({


Might be good to mention here why a new object is required rather than mutating the message. It took me a second to realize that manipulations here would manipulate the original action object which could cause unintentional side-effects.

Will add a comment.

ethanroday · 2018-03-23T00:31:25Z

The '' to null change caused one of my tests to fail since it was expecting chrome.runtime.sendMessage to have been called with ('', ...). Should be good now.

tshaddix · 2018-03-23T00:47:12Z

@vhmth would love a second pass from you! This is a pretty big change!

tshaddix · 2018-04-07T16:37:59Z

@vhmth I'm tempted to merge this and #124 and release under a prerelease as they are fairly large changes. Thoughts?

vhmth · 2018-04-16T00:42:05Z

index.d.ts

@@ -9,6 +9,8 @@ export class Store<S = any, A extends redux.Action = redux.Action> {
    portName: string,
    state?: any,
    extensionId?: string,
+    serializer?: Function,


thank you for adding the TS defs

vhmth · 2018-04-16T00:43:02Z

src/serialization.js

+  // just return a copy of the message.
+  // We return a copy rather than the original message so that we're not
+  // mutating the original action object.
+  ...(message.payload ? {payload: transformer(message.payload)} : {})


Very nice - love the cascading spread

vhmth · 2018-04-16T00:59:29Z

test/Store.test.js

@@ -255,6 +298,21 @@ describe('Store', function () {
      }).should.eql(true);
    });

+    it('should serialize payloads before sending', function () {


These tests are incredible. Mind adding one more test that serializes/deserializes a date object and checks the input's (date originally stored in action) getDate to the output's (date received after deserialization)? Another check in this test that would be good is checking that the two date objects are not triple-equal to each other to confirm that serialization/deserialization indeed happened.

Missed the boat on this one, but hopefully the existing tests are sufficient. If it helps, I've been using this in production for a couple of months and all seems well.

vhmth · 2018-04-16T00:59:40Z

test/Store.test.js

@@ -294,4 +346,24 @@ describe('Store', function () {
      return p.should.be.fulfilledWith(undefined);
    });
  });
+
+  describe("when validating options", function () {


very very nice!

vhmth · 2018-04-16T01:02:31Z

@tshaddix @ethanroday

https://www.useloom.com/share/86d043f30c5a4515be7238b2909b4fc3

vhmth

Other than my comment about adding another test, everything else makes so much sense. This PR is awesome!

tshaddix · 2018-04-16T15:36:16Z

Great! Let's finalize this thing!

tshaddix · 2018-04-16T15:39:27Z

@vhmth Given this, I'm thinking we just release under a next tag on NPM. Thoughts?

Internal tag would be v1.6.0-alpha.1 in GitHub. I don't think we need to increment a major version as this doesn't change the API from the past.

vhmth · 2018-04-16T17:52:27Z

Absolutely down. My vote would be to actually increase by a major version but seems like I missed the boat on that one! Ah well - cool with 1.6.0-alpha.1 anyhow.

ethanroday · 2018-04-16T23:21:35Z

Thanks for the shoutout, @vhmth! Happy to be contributing to something that's been so useful to me 😄

tshaddix · 2018-04-17T14:53:40Z

@vhmth We can always bump it to 2.0 alpha! Why would you vote major, just curious?

@ethanroday Yes, thank you very much for your fantastic contribution. I echo @vhmth 's sentiment completely. Regarding the additional test, you can always add it as a separate PR :)

vhmth · 2018-04-20T00:03:45Z

@tshaddix primary reason for the bump to major is I look at "plug and play" layers as a step towards increasing complexity in many ways. In this case we've introduced a custom serialization that diverges away from the standard Chrome behavior. I smell lots of issues potentially coming up since this interferes with the core communication mechanism b/w background and content processes.

tshaddix · 2018-04-20T16:03:25Z

@vhmth Ah, I see your point and I see my point 😄

I'm happy to bump major when on next publish.

vhmth · 2018-04-21T21:07:12Z

@tshaddix that works!

Ethan Roday added 12 commits January 11, 2018 16:54

Introduce withSerializer and withDeserializer and expose as options t…

403c209

…o Store and wrapStore

Upgrade Babel to mono-repo and add stage 1 preset

a3fd0c1

Add shouldDeserialize and beef up tests

60822fd

Add shouldDeserialize and integration tests for wrapStore

afd4040

Fix a broken test

53387fd

Integration tests for Store

739cbad

Update README

a73b30c

Update typings

1e61b1f

Typings fix

944f165

Fix linting errors

ab1ca4d

Revert "Upgrade Babel to mono-repo and add stage 1 preset"

1cfd11d

This reverts commit a3fd0c1.

Add stage-1

ddcca26

ethanroday mentioned this pull request Jan 12, 2018

Serialization hooks for Dates #125

Closed

Wrap all chrome function references

95db373

tshaddix requested review from vhmth and tshaddix January 14, 2018 19:39

Ethan Roday added 2 commits January 22, 2018 13:06

Quick fix on documentation

4802bbd

Return result of listener for async event handling

0d32f77

Merge branch 'master' into serialization-hooks

00f2378

Merge branch 'master' into serialization-hooks

9cf69c8

tshaddix requested changes Mar 22, 2018

View reviewed changes

Add comment and fix failing test

b26774f

tshaddix approved these changes Mar 23, 2018

View reviewed changes

vhmth reviewed Apr 16, 2018

View reviewed changes

vhmth approved these changes Apr 16, 2018

View reviewed changes

tshaddix merged commit 306f6de into tshaddix:master Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialization hooks #126

Serialization hooks #126

ethanroday commented Jan 12, 2018 •

edited

Loading

vhmth commented Jan 15, 2018

ethanroday commented Jan 18, 2018

ethanroday commented Jan 23, 2018

ethanroday commented Feb 9, 2018

tshaddix commented Feb 14, 2018

tshaddix left a comment

tshaddix Mar 22, 2018

ethanroday Mar 23, 2018 •

edited

Loading

tshaddix Mar 23, 2018

vhmth Apr 16, 2018

tshaddix Mar 22, 2018

vhmth Apr 16, 2018

tshaddix Mar 22, 2018

ethanroday Mar 23, 2018

ethanroday commented Mar 23, 2018

tshaddix commented Mar 23, 2018

tshaddix commented Apr 7, 2018

vhmth Apr 16, 2018

vhmth Apr 16, 2018

vhmth Apr 16, 2018

ethanroday Apr 16, 2018 •

edited

Loading

vhmth Apr 16, 2018

vhmth commented Apr 16, 2018

vhmth left a comment

tshaddix commented Apr 16, 2018

tshaddix commented Apr 16, 2018 •

edited

Loading

vhmth commented Apr 16, 2018

ethanroday commented Apr 16, 2018

tshaddix commented Apr 17, 2018

vhmth commented Apr 20, 2018

tshaddix commented Apr 20, 2018

vhmth commented Apr 21, 2018

		@@ -0,0 +1,91 @@
		export const noop = (payload) => payload;

		const transformPayload = (message, transformer = noop) => ({

Serialization hooks #126

Serialization hooks #126

Conversation

ethanroday commented Jan 12, 2018 • edited Loading

vhmth commented Jan 15, 2018

ethanroday commented Jan 18, 2018

ethanroday commented Jan 23, 2018

ethanroday commented Feb 9, 2018

tshaddix commented Feb 14, 2018

tshaddix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanroday Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanroday commented Mar 23, 2018

tshaddix commented Mar 23, 2018

tshaddix commented Apr 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanroday Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vhmth commented Apr 16, 2018

vhmth left a comment

Choose a reason for hiding this comment

tshaddix commented Apr 16, 2018

tshaddix commented Apr 16, 2018 • edited Loading

vhmth commented Apr 16, 2018

ethanroday commented Apr 16, 2018

tshaddix commented Apr 17, 2018

vhmth commented Apr 20, 2018

tshaddix commented Apr 20, 2018

vhmth commented Apr 21, 2018

ethanroday commented Jan 12, 2018 •

edited

Loading

ethanroday Mar 23, 2018 •

edited

Loading

ethanroday Apr 16, 2018 •

edited

Loading

tshaddix commented Apr 16, 2018 •

edited

Loading