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

Alternative API #26

Merged
merged 2 commits into from
Oct 30, 2017
Merged

Alternative API #26

merged 2 commits into from
Oct 30, 2017

Conversation

wacii
Copy link
Collaborator

@wacii wacii commented Sep 16, 2017

Currently, Redux Offline decides where to insert the offline middleware. This is unnecessary and can sometimes cause difficulties with other libraries, as in #16.

This PR introduces an alternative API, for when users need a bit more flexibility.

Other changes are possible. The reducer could be exposed instead of enhanceReducer(). And there is really no reason to set up Redux Persist for the user: it just forces them to use a particular version of the library. But I figured this is enough code to start a discussion.

Not immediately important, but something to consider.

@sorodrigo sorodrigo changed the base branch from master to develop September 19, 2017 03:19
@sorodrigo
Copy link
Owner

This is great! I need time to test it, but looks awesome!

@jsslai
Copy link

jsslai commented Oct 17, 2017

@wacii

Other changes are possible. The reducer could be exposed instead of enhanceReducer()

This would be great. Now it is not possible to use custom structure for the Redux state.

@wacii
Copy link
Collaborator Author

wacii commented Oct 17, 2017

@jsslai #42 introduced offlineStateLens() so that the offline state could be stored in an immutable object, but it could also be used to change where the state is stored. That change is currently in the dev branch and will be available next release.

This should probably be mentioned in the documentation... I'll add it to my TODOs.

@jsslai
Copy link

jsslai commented Oct 18, 2017

@wacii Thanks! I’ll check that.

@jaulz
Copy link

jaulz commented Oct 21, 2017

Nice Work. Any time plans for the next release? :)

@sorodrigo
Copy link
Owner

sorodrigo commented Oct 29, 2017

Hey @wacii ! I tried merging this with the current develop branch, but I'm facing some issues with the changes introduced when we added immutable support.
Besides that like you mentioned earlier:

#42 introduced offlineStateLens() so that the offline state could be stored in an immutable object, but it could also be used to change where the state is stored.

So I'm wondering does this still make sense?

If it does, I need some help on updating createOffline to use the new immutable store implementation. I've been playing around with it, but haven't gotten any interesting progress. I could push my latest changes (not working) or leave the code as it is after merging if you prefer.

@wacii
Copy link
Collaborator Author

wacii commented Oct 29, 2017

@sorodrigo It does still make sense. The main purpose of this was to give the user control over where the middleware was applied.

I'd just leave the code as is. I'll work on this tonight.

@sorodrigo
Copy link
Owner

great and thanks! I also added a npm script for running the alternative API in the examples. As of now, because the package.json is pointing to npm this won't work out of the box, but if you want to test it just need to point it to your git branch.

@wacii
Copy link
Collaborator Author

wacii commented Oct 29, 2017

Rebased against develop. I cherry-picked your addition.

@sorodrigo
Copy link
Owner

works like a charm!

@sorodrigo sorodrigo merged commit 3a0684a into sorodrigo:develop Oct 30, 2017
@wacii wacii mentioned this pull request Nov 11, 2017
@wacii wacii deleted the alternative-api branch February 20, 2018 19:34
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.

4 participants