Implement initial default adapter for upcoming adapter pattern #51

heyellieday · 2017-11-13T19:27:14Z

Summary

This PR includes changes to the model and collection actions present in this repo. Specifically, an "adapter pattern" has been introduced to allow the invoker of the above-mentioned actions to provide an adapter that responds to a standard set of method calls, like adapter.fetchCollection or adapter.saveModel with the purpose of syncing records with a remote brainstem backend.
Usage of this pattern moves the process of syncing from within these model and collection actions to a given adapter.

The motivation for this change is to allow usage of brainstem-redux without the involvement of StorageManager/Backbone. Using Backbone models through StorageManager is a detriment to application performance when a large number of records are loaded into the front-end. This change will allow Mavenlink (or others) to write a custom adapter that bypasses StorageManager and loads many records directly into state.brainstem in a more performant manner.

While this is a substantial change to how brainstem-redux can function, all existing functionality provided by this library will be preserved as the default behavior. This is achieved by providing a default adapter to each action that calls the existing StorageManager-specific code. So while it is possible to use a custom adapter, the backbone-centric implementation will continue to the default option, ensuring backwards compatibility.

Test plan

This behavior is verified through tests that verify the existing StorageManager-specific code is called when an adapter is not passed in as an option and that a passed in adapter has it's methods called as expected. With the exception of the defaultAdapter that includes storage manager code, we don't care how the adapter implements its syncing, but instead that it responds to calls in the expected ways. Adapters will be responsible for unit testing their syncing functionality.

General upkeep checklist

Examples are working

Signed-off-by: Brandon Duff <[email protected]>

juanca · 2017-11-13T21:39:33Z

Great PR description! 💯

heyellieday · 2017-11-14T00:49:02Z

spec/actions/collection-spec.js


-      expect(abort).toHaveBeenCalledTimes(0);
+      expect(stubAdapter.fetchCollection).toHaveBeenCalledWith('posts', {
+        dispatch: jasmine.any(Function),


If anyone looking this code over has a good idea for testing that correct dispatch and getState were passed into this and every other action, feel free to suggest it. This is my solution for now since it turns out the dispatch passed in from the param of a called thunk is not the same as the dispatch that is evoked on line 81 above.

We could also call this.fetch(arguments)(dispatchSpy, getStateSpy) and that should do the trick, but I don't know if that's needed.

Yeah, I think we're fine for now. Appreciate the comment, it will be good for historical context if it is ever dug up.

- pass the adapter to collection fetch from make_brainstem_type - fix code formatting issues around indentations and trailing commas Signed-off-by: Ellie Day <[email protected]>

Signed-off-by: Bobby Brown <[email protected]>

… state - fix lint error around lengthy line of code in make brainstem type Signed-off-by: Ellie Day <[email protected]>

…ded methods in model actions for adapter pattern

juanca · 2017-11-21T00:38:57Z

spec/actions/model-spec.js

@@ -205,7 +205,7 @@ describe('model action creators', () => {
        this.destroy = modelActions.destroy;
      });

-      it('calls destroy on the adapter', function () {
+      it('calls destroy on the adapter', () => {


uh, bad merge conflict? Let's not nest it blocks!

juanca · 2017-11-21T00:39:47Z

spec/actions/model-spec.js

-      const xhr = dispatch();
-      expect(xhr.done).toEqual(jasmine.any(Function));
-      expect(xhr.fail).toEqual(jasmine.any(Function));
+      it('calls destroy on the adapter', () => {


What is going on here.

There are nested it specs in it specs. Can we fix that?

juanca · 2017-11-21T00:39:52Z

spec/actions/model-spec.js

+        this.destroy = modelActions.destroy;
+      });
+
+      it('calls destroy on the adapter', () => {


What is going on here.

To reply to your past few comments re: "what is going on here", I'd suggest looking at the diff side by side as it will be easier to see that the existing specs were wrapped in an additional describe block. This causes the diff to appear a bit wonky, which you've likely seen before. I hope that clears things up!

There are nested it specs in it specs. Can we fix that?

Ah, I see what you mean. Fixed it!

heyellieday · 2017-11-21T00:57:54Z

@juanca, this PR is nearly ready for a review, so I'll be sure to ping you in the next day or so when it's all cleaned up. There aren't plans to add much more to this PR. If you're curious, an adapter implementation is in progress here: https://github.com/mavenlink/mavenlink/pull/10353

heyellieday · 2017-11-27T18:26:32Z

@juanca, this is ready for a final look over. I've published a beta with a version of 0.0.35-beta.1 that is being used in this PR: https://github.com/mavenlink/mavenlink/pull/10353

juanca · 2017-11-27T21:48:02Z

Can you verify that the examples are working?

juanca · 2017-11-27T21:49:18Z

lib/actions/collection.js


-      if (postFetchAction) xhr.done(collection => dispatch(postFetchAction(collection.pluck('id'))));
+      if (postFetchAction) {
+        xhr.done(collection =>


How is .done going to work when the XHR is not from jQuery?

We have our mavenlink adapter return a deferred object like so:

fetchCollection: (brainstemKey, { dispatch, fetchOptions }) => { const deferred = $.Deferred(); networkHelpers.fetchFromAPI(baseUrl, { params: fetchOptions.params, method: 'GET' }) .then((result) => { dispatch(brainstemActions.syncCollections(result)); deferred.resolve(result[brainstemKey]); }); return deferred; },

juanca · 2017-11-27T21:50:38Z

lib/actions/collection.js


      if (trackKey) xhrs[trackKey] = xhr;

      return new Promise((resolve, reject) => xhr
-        .done(collection => resolve(collection.map(model => model.attributes)))
+        .done(collection => resolve(adapter.collectionToArray(collection)))


What's the use-case for overriding collection.map? EDIT: And I'm assuming it's the same for overriding model.get('id')?

We are delegating the handling of items in a collection to the adapter. Brainstem redux is now indifferent to the shape of the collection being passed in.

juanca · 2017-11-27T21:56:06Z

lib/actions/collection.js

@@ -1,4 +1,4 @@
-const { StorageManager } = require('brainstem-js');


Removing the dependency to the StorageManager is fantastic! I know it's outta scope so I made a follow-up issue about removing it from the defaults as well:

#52

juanca · 2017-11-27T22:01:34Z

lib/adapters/default.js

+  fetchCollection: (brainstemKey, options) => {
+    const storageManager = StorageManager.get();
+    return storageManager.storage(brainstemKey)
+      .fetch(Object.assign(options.fetchOptions, { remove: false }));


We should never be modifying anything we don't own -- unless you can guarantee that the object is thrown away.

Let's make sure we're cloning and creating a new object.

Oh, I see that's how it was before. Let's still make the edit -- it happens that the previous code had a mistake.

juanca · 2017-11-27T22:06:36Z

lib/reducers/sync-collections.js

+
+  collectionNames.forEach((collectionName) => {
+    mergedCollections[collectionName] = {
+      ...state[collectionName],


I don't think we've added ES6 spreads as dependencies.

Let's switch these to Object.assign.

juanca · 2017-11-27T22:27:53Z

spec/types/make-brainstem-type-spec.js

@@ -114,10 +114,21 @@ describe('makeBrainstemType', () => {
    });

    it('forwards the request to brainstem-redux', () => {
-      const options = 'OPTIONS';
+      const options = { stuff: 'options' };


This teaches me a lesson to use stubbed data objects that are similar to the real life object.

Juanca = 👨‍🎓
(says Bobby)

juanca · 2017-11-27T22:33:58Z

spec/actions/model-spec.js

+        this.save = modelActions.save;
+      });
+
+      it('send save to the subscriber for the existing model', function () {


…ginal objects Signed-off-by: Ellie Day <[email protected]>

Signed-off-by: Bobby Brown <[email protected]>

Signed-off-by: Ellie Day <[email protected]>

juanca · 2017-11-27T23:29:45Z

lib/types/make-brainstem-type.js

@@ -7,9 +7,16 @@ const defaultTypeOptions = {
 };

 module.exports = function makeBrainstemType(brainstemKey, typeOptions = defaultTypeOptions) {
+  const mergedOptions = { ...defaultTypeOptions, ...typeOptions };


Object.assign and the rest of the spreads please.

juanca

Contingent on no usage of ... operators

…isting object Signed-off-by: Bobby Brown <[email protected]>

…ainstem-redux into add_adapter_pattern

…redux into add_adapter_pattern

Signed-off-by: Ellie Day <[email protected]>

Make options object as second param of makeBrainstemType

Signed-off-by: Ellie Day <[email protected]>

Re-merge to correctly merge adapter pattern (PR #51) into master

Ellie Day added 2 commits November 9, 2017 17:00

Add initial adapter pattern for collection.js

c9c2a13

Signed-off-by: Brandon Duff <[email protected]>

Add remaining adapter methods to existing actions

396fded

Ellie Day added 5 commits November 13, 2017 14:10

Fix linting

594737f

Move storageManager actions to shared adapter in in adapters/default.js

eb7f531

Add missing adapters foldee to git

4dda476

revert to basic function pattern

daf3c01

Make dispatch and getState available as options to all adapter actions

f3c7017

heyellieday commented Nov 14, 2017

View reviewed changes

Bobby Brown and others added 4 commits November 14, 2017 16:04

Added collectionToIds and collectionToArray to the default adapter

ab39813

- pass the adapter to collection fetch from make_brainstem_type - fix code formatting issues around indentations and trailing commas Signed-off-by: Ellie Day <[email protected]>

Conditionally pass in adapter when its defined in typeOptions

5807638

Signed-off-by: Bobby Brown <[email protected]>

Add the SYNC_COLLECTIONS reducer that merges new collections into the…

3ee071f

… state - fix lint error around lengthy line of code in make brainstem type Signed-off-by: Ellie Day <[email protected]>

add additional makeBrainstemType actions that use adapter and add nee…

4fe6296

…ded methods in model actions for adapter pattern

juanca reviewed Nov 21, 2017

View reviewed changes

Ellie Day added 3 commits November 20, 2017 17:55

fix issues with validate + destroy specs

31f9d98

move option building to shared method

2656bf3

bump version to next beta

5e50c93

juanca changed the title ~~[WIP] Implement initial default adapter for upcoming adapter pattern~~ Implement initial default adapter for upcoming adapter pattern Nov 27, 2017

juanca reviewed Nov 27, 2017

View reviewed changes

Bobby Brown and others added 3 commits November 27, 2017 14:37

Prefer Object.assign with an empty object to prevent mutating the ori…

a0ea814

…ginal objects Signed-off-by: Ellie Day <[email protected]>

Fix typo in spec assertion

ab98fe0

Signed-off-by: Bobby Brown <[email protected]>

Add directions for running and testing example code in README

201b48f

Signed-off-by: Ellie Day <[email protected]>

juanca reviewed Nov 27, 2017

View reviewed changes

juanca approved these changes Nov 27, 2017

View reviewed changes

Ellie Day and others added 6 commits November 27, 2017 16:34

Use Object.assign with a new empty object in favor of mutating the ex…

a1be14f

…isting object Signed-off-by: Bobby Brown <[email protected]>

Merge branch 'add_adapter_pattern' of https://github.com/mavenlink/br…

2785ac7

…ainstem-redux into add_adapter_pattern

Merge branch 'add_adapter_pattern' of github.com:mavenlink/brainstem-…

b0d6aba

…redux into add_adapter_pattern

Bump version number to 0.0.35

424dd71

Signed-off-by: Ellie Day <[email protected]>

Merge pull request #50 from mavenlink/make_brainstem_type_options

1c56c06

Make options object as second param of makeBrainstemType

Merge branch 'master' into add_adapter_pattern

e13303e

Signed-off-by: Ellie Day <[email protected]>

heyellieday merged commit ca8975c into make_brainstem_type_options Nov 28, 2017

heyellieday deleted the add_adapter_pattern branch November 28, 2017 00:56

heyellieday mentioned this pull request Dec 6, 2017

Re-merge to correctly merge adapter pattern (PR #51) into master #53

Merged

heyellieday added a commit that referenced this pull request Dec 6, 2017

Merge pull request #53 from mavenlink/make_brainstem_type_options

3d396f1

Re-merge to correctly merge adapter pattern (PR #51) into master

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement initial default adapter for upcoming adapter pattern #51

Implement initial default adapter for upcoming adapter pattern #51

heyellieday commented Nov 13, 2017

juanca commented Nov 13, 2017

heyellieday Nov 14, 2017 •

edited

Loading

juanca Nov 27, 2017

juanca Nov 21, 2017

juanca Nov 21, 2017

juanca Nov 21, 2017

juanca Nov 21, 2017

heyellieday Nov 21, 2017

juanca Nov 21, 2017

heyellieday Nov 21, 2017

heyellieday commented Nov 21, 2017

heyellieday commented Nov 27, 2017

juanca commented Nov 27, 2017

juanca Nov 27, 2017

heyellieday Nov 27, 2017

juanca Nov 27, 2017 •

edited

Loading

heyellieday Nov 27, 2017

juanca Nov 27, 2017 •

edited

Loading

juanca Nov 27, 2017

juanca Nov 27, 2017 •

edited

Loading

juanca Nov 27, 2017

juanca Nov 27, 2017

heyellieday Nov 27, 2017 •

edited

Loading

juanca Nov 27, 2017

juanca Nov 27, 2017

juanca left a comment

		@@ -1,4 +1,4 @@
		const { StorageManager } = require('brainstem-js');

Implement initial default adapter for upcoming adapter pattern #51

Implement initial default adapter for upcoming adapter pattern #51

Conversation

heyellieday commented Nov 13, 2017

juanca commented Nov 13, 2017

heyellieday Nov 14, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyellieday commented Nov 21, 2017

heyellieday commented Nov 27, 2017

juanca commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanca Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanca Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanca Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyellieday Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanca left a comment

Choose a reason for hiding this comment

heyellieday Nov 14, 2017 •

edited

Loading

juanca Nov 27, 2017 •

edited

Loading

juanca Nov 27, 2017 •

edited

Loading

juanca Nov 27, 2017 •

edited

Loading

heyellieday Nov 27, 2017 •

edited

Loading