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

Resetting services reducers doesn't allow queryResult to be set to an arbitrary value #36

Open
rafacv opened this issue Oct 27, 2017 · 3 comments

Comments

@rafacv
Copy link

rafacv commented Oct 27, 2017

Hello,

first of all I'd like to thank you guys for all the hard work on feathers-plus repos. I'm using feathers-redux in a project and it's really great. 👏

Recently I came across what I believe to be an unexpected behavior. I thought about submitting a PR, but first I'd like to clarify my issue with you and see if it's really unintended behavior.

Code to be executed

dispatch(reduxServices.serviceName.reset(payload));

Expected behavior

I'd expect the reducer to be updated to hold the payload passed as argument in queryResult. Like:

          {
            isError: null,
            isLoading: false,
            isSaving: false,
            isFinished: false,
            data: null,
            queryResult: payload,
            store: null
          }

Actual behavior

Instead queryResult is always kept to whatever it was set before the call to reset method.

          {
            [...]
            queryResult: queryResultPreviousValue,
            [...]
          }

Code in question

https://github.com/feathers-plus/feathers-redux/blob/6a10bd9/src/index.js#L197-L214

In Redux, all actions are dispatched passing to reducers their current state along with the action object (with its payload, type, and whatnot). With that in mind, I think the following line:

[opts.queryResult]: action.payload ? state[opts.queryResult] : null,

Should rather be:

[opts.queryResult]: action.payload,

So we set queryResult to whatever is passed to the reset method call allowing coders to set arbitrary values to it. state[opts.queryResult] will always keep the same value queryResult is already defined to.

Am I wrong in my assumptions or analysis?
Let me know if I can clarify or elaborate on anything.

Thank you!!!

@rafacv
Copy link
Author

rafacv commented Oct 27, 2017

I just read this test and realized my expectation was wrong:
https://github.com/feathers-plus/feathers-redux/blob/master/test/reducer.test.js#L131-L144

Reset method expects either no argument or true. In case of the latter, the previous value is kept instead of erased.

What I'm trying to accomplish is: in case my call to the remote service fails for whatever reason, I provide a sensible default. I'm sorry to turn issue in a Q&A, but is there a standard way to define values for the reducers if not through reset? Manually dispatch a SERVICES_MYSERVICE_FIND action with a specially crafted payload seems wrong.

Thank you.

@rafacv
Copy link
Author

rafacv commented Oct 27, 2017

Ok, so my main concern was to have a dumb container that doesn't need to know anything about failing services since this service in particular is not a deal breaker if not available at the time of request. Instead of trying to forcefully set values in the reducer to provide default values, I'm having a higher order component to deal with it passing down props as appropriate whether it's a success or a failure.

Sorry for taking your time with my ignorance. Closing issue.

@rafacv rafacv closed this as completed Oct 27, 2017
@eddyystop
Copy link
Collaborator

eddyystop commented Oct 28, 2017

The README states reset has no params.

// action creators
    create(data, params) {}, // Action creator for app.services('messages').create(data, params)
    update(id, data, params) {},
    patch(id, data, params) {},
    remove(id, params) {},
    find(params) {},
    get(id, params) {},
    store(object) {}, // Interface for realtime replication.
    reset() {}, // Reinitializes store for this service.

However there's an argument for reset(data) or reset(data, options). What was your use case?

@eddyystop eddyystop reopened this Oct 28, 2017
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

No branches or pull requests

2 participants