feathersjs-ecosystem / feathers-redux

Integrate Feathers with your Redux store
MIT License
114 stars 23 forks source link

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

Open rafacv opened 6 years ago

rafacv commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

eddyystop commented 6 years ago

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?