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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Redux store enhancer API #17

Closed
wants to merge 3 commits into from
Closed

Use Redux store enhancer API #17

wants to merge 3 commits into from

Conversation

migueloller
Copy link
Contributor

@migueloller migueloller commented Mar 29, 2017

This PR implements the Redux store enhancer API as requested in #3.

This would be a breaking change because the API would now be the following:

-import { applyMiddleware, createStore } from 'redux';
+import { applyMiddleware, createStore, compose } from 'redux';
+import { offline } from 'redux-offline';
+import offlineConfig from 'redux-offline/lib/defaults';

 // ...

 const store = createStore(
   reducer,
   preloadedState,
-  applyMiddleware(middleware)
+  compose(offline(offlineConfig), applyMiddleware(middleware))
 );

The names used can definitely be changed. This is just an initial POC to show how easy it is to implement.

If help is needed for better Flow types, I wouldn't mind helping out with that as well.

Let me know what you think, @jevakallio!

馃嵑

@jevakallio jevakallio mentioned this pull request Apr 5, 2017
@timbuckley
Copy link

timbuckley commented Apr 5, 2017

The API can be backwards-compatible and non-breaking if you simply expose both the new offlineEnhancer and createOfflineStore.

import {
  offlineEnhancer, // New enhancer now exposed.
  createOfflineStore, // Still exposed but deprecated.
} from 'redux-offline';

Therefore, both the new and the old forms would work:

Old way:

- import { applyMiddleware, createStore } from 'redux';
+ import { applyMiddleware } from 'redux';
+ import { createOfflineStore } from 'redux-offline';
+ import offlineConfig from 'redux-offline/lib/defaults';

- const store = createStore(
+ const store = createOfflineStore(
  reducer,
  preloadedState,
  applyMiddleware(middleware),
+ offlineConfig  
);

New way:

-import { applyMiddleware, createStore } from 'redux';
+import { applyMiddleware, createStore, compose } from 'redux';
+import { offlineEnhancer } from 'redux-offline';
+import offlineConfig from 'redux-offline/lib/defaults';

 const store = createStore(
   reducer,
   preloadedState,
-  applyMiddleware(middleware)
+  compose(offlineEnhancer(offlineConfig), applyMiddleware(middleware))
 );

@jevakallio
Copy link
Collaborator

jevakallio commented Apr 6, 2017

Thanks for the input @timbuckley! I don't mind doing breaking changes as long as we follow semver, and would prefer to keep just one canonical API.

However, attempting to test and integrate this PR, I did run into some trouble, and wasn't able to get the user's provided enhancers compose with the middleware enhancer created by redux-offline.

I have not had time to further investigate since. I will get back to this ASAP, but just recording here that the breaking change is not a blocker for merging this.

@migueloller does you branch with these changes and the proposed consumer API work for you?

@migueloller
Copy link
Contributor Author

@jevakallio,

Thanks for the reply! Do you want any help with this? Also, with regards to my Flow comments, is that something you would like contributions for?

@jevakallio
Copy link
Collaborator

jevakallio commented Apr 6, 2017

@migueloller if you are able to get the store enhancer API working (or just convince me that it already works and I'll give this PR another shot), that'd be great.

Improving flowtypes would be a great contribution, as a separate PR.

My plan is to refactor the updater/actions to make the actions exposed by the library public and more explicit, so people wo want to use their own effects middleware (e.g. sagas) instead of the network reconciler included in redux-offline can replicate the retry behaviour with them.

I have, however, grossly overestimated the time I have available for OSS on top of my day job, so things are not moving forwards. Any help is very much appreciated! 馃嵑

@migueloller
Copy link
Contributor Author

@jevakallio,

I decided to take a look at why this would be happening and found a bug in the current implementation (that stayed on after my changes).

If there is no enhancer passed to createOfflineStore then Redux will throw an error as enhancer will be undefined. Take a look at these lines where enhancer will be undefined if not passed to the function. And then a look at this line where you will get the following error if enhancer is undefined:

~/redux-offline/node_modules/redux/lib/compose.js:35
      return f(composed);
             ^

TypeError: f is not a function
    at ~/redux-offline/node_modules/redux/lib/compose.js:35:14

The solution is simply to check if enhancer is undefined (or give it a default value of id, where id is const id = x => x).

I'll make this change, rebase against master and commit again.

This will fix enhancers for both the current API and this new API.

@migueloller
Copy link
Contributor Author

@jevakallio,

With regards to other ergonomic improvements like Flow types and such, I will try to contribute if I find some time, but like you, my day job is keeping me busy! 馃槄

@jevakallio
Copy link
Collaborator

@migueloller I cherry-picked commits e32ef99 and 81c205a to master and released them as v2.0.0 along with some documentation updates.

Thanks for your help 馃嵒!

@jevakallio jevakallio closed this Apr 14, 2017
@msmaromi
Copy link

@jevakallio so, how to use sagas effect in offline meta?

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

Successfully merging this pull request may close these issues.

None yet

4 participants