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

Return structure of selectRecord suboptimal #68

Open
oertels opened this issue Jan 24, 2018 · 3 comments
Open

Return structure of selectRecord suboptimal #68

oertels opened this issue Jan 24, 2018 · 3 comments

Comments

@oertels
Copy link

oertels commented Jan 24, 2018

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
}
@devvmh
Copy link
Owner

devvmh commented Jan 24, 2018

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.

@oertels
Copy link
Author

oertels commented Jan 24, 2018

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 #38 up-to-date and add this feature request.

@devvmh
Copy link
Owner

devvmh commented Jan 25, 2018

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.

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