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

Always add response headers to the redux store #84

Merged

Conversation

hobbeswalsh
Copy link
Contributor

Hey there,

I'm not sure what the original reason was to exclude response headers from the store, but I think there is a strong case for having them enabled by default. This PR implements that, but I welcome feedback about original design decisions (or about a way to make this behavior optional, if that's preferred).

Thanks!

@ryanashcraft
Copy link
Contributor

Hi @hobbeswalsh. Generally we just try to limit what goes in the reducers as much as possible for (a) API simplicity and (b) to avoid retaining too much memory. I can see the response headers possibly being useful. What is your use case? What headers do you need for what purpose? Would like to think more about this before deciding one way or another.

Also you have a typo in your change: repsonseHeaders. In addition, if we do decide to accept this change, you should also add a query selector in selectors/query.js. Also I need to fix the lint errors that are failing the travis build.

@hobbeswalsh
Copy link
Contributor Author

I can see the response headers possibly being useful. What is your use case?

Our particular use-case is that we have pagination tokens coming back in the headers. It would probably make our job of paginating easier, not to mention being able to correlate the body of the response in the store with the corresponding header (and pagination token).

Also you have a typo in your change: repsonseHeaders. In addition, if we do decide to accept this change, you should also add a query selector in selectors/query.js.

Oops, sorry, and thanks. I won't fix this until you get some time to think about whether or not the benefits of this change will outweigh the memory costs. If you decide you want to keep it, I'll fix the typo, add a selector, and potentially a unit test or two as well.

Thanks for the response!

@ryanashcraft
Copy link
Contributor

@hobbeswalsh Makes sense to me! Will merge this with the requested changes.

@hobbeswalsh
Copy link
Contributor Author

Sorry, @ryanashcraft, I don't think I'm able to resolve these conflicts, but I think the changes are all in.

@ryanashcraft ryanashcraft merged commit c490b07 into amplitude:master Oct 31, 2017
@hobbeswalsh
Copy link
Contributor Author

hobbeswalsh commented Oct 31, 2017 via email

@ryanashcraft
Copy link
Contributor

Thanks @hobbeswalsh for the contribution! Will publish a new version soon.

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.

2 participants