redux-crud-store icon indicating copy to clipboard operation
redux-crud-store copied to clipboard

Return structure of selectRecord suboptimal

Open oertels opened this issue 7 years ago • 3 comments

Hi,

is there a reason selectRecord() returns either a Selection or the server response? There could be collisions, if the server response has e.g. the field isLoading or error.

I think the return value could be simplified by not returning sel.data but the whole object. In this case, you always know what you're dealing with.

And last, I don't understand why the error field contains an Error object when in loading state. Shouldn't the error be empty then?

In summary, I'd propose an object structure like this:

return {
    isLoading: boolean,
    needsFetch: boolean,
    error?: Error,
    data?: T
}

oertels avatar Jan 24 '18 17:01 oertels

Thanks for this issue! The updates to the Flow spec of the return type sounds like a positive change.

I'm more hesitant about removing data envelopes from the lib. In hindsight, it would certainly simplify the code, but it would push a bit more work onto the users. One goal of this library is that if you have a reasonably normal API (including APIs with and without data envelopes), this should work without too much configuration. Anything crazier and you're expected to implement the parsing logic using a custom ApiClient or something else.

Anyways, if you write this up as a pull request I'm happy to review and merge.

devvmh avatar Jan 24 '18 17:01 devvmh

I don't see how this would complicate things, rather than making them easier. If you can be sure that you've got always a isLoading, and, if not loading, a data field, and that there's only an error field in case of an error, it appears more transparent to me. Since this would break backwards compatibility, I assume it's more a feature for a new major version. Maybe it's a good opportunity to bring https://github.com/devvmh/redux-crud-store/issues/38 up-to-date and add this feature request.

oertels avatar Jan 24 '18 20:01 oertels

I agree it's a major simplification to the logic of the lib.

But for users, it pushes just a little bit more work onto them. I'd rather see a PR that improves the output of the selectors while maintaining its ability to "parse" data envelopes.

devvmh avatar Jan 25 '18 03:01 devvmh