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

Add Support for Identity Requests #36

Merged
merged 5 commits into from
Jan 16, 2016
Merged

Add Support for Identity Requests #36

merged 5 commits into from
Jan 16, 2016

Conversation

ryanbrainard
Copy link
Contributor

This implements "identity requests" as described in #33. See Identity Requests: Static Data & Transforming Responses for an explanation and example.

Note, this is not meant to handle the use case in #18 for post-processing (e.g. handling CSV responses) options. There is surely some overlap, but this is for transforming responses after any post-processing.

cc: @jsullivan @hburrows @georgebonnr @ashtuchkin @hnordt

@ryanbrainard ryanbrainard force-pushed the identity-requests branch 2 times, most recently from 4357770 to a8608be Compare January 11, 2016 08:03
…y Requests: Static Data & Transforming Responses](https://github.com/heroku/react-refetch/blob/identity-requests/README.md#identity-requests-static-data--transforming-responses) for an explanation and example.

Note, this is not meant to handle the use case in #18 for post-processing (e.g. handling CSV responses) options. There is surely some overlap, but this is for transforming responses after any post-processing.

mapping.equals = function (that) {
if (this.comparison !== undefined) {
return this.comparison === that.comparison
}

return [ 'url', 'method', 'headers', 'body' ].every((c) => {
return [ 'value', 'url', 'method', 'headers', 'body' ].every((c) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanbrainard Not 100% familiar with the code... But if you have value: doTransformationWork(rawValue) in your mapping, that work will be done every time to produce the final value, so the equality check here doesn't prevent that work from being done?

That being said, seems like the primary use case for the value prop is in a then or andThen prop (so the equality check for the parent mapping is the important part)... This could be used in a 'top level' mapping, but then you'd probably just want to apply any transformations to the plain props directly instead of using refetch?

Is that correct or am I missing something? Going to be using this in .then so not really a problem. But I guess I can also use my own memoization on that transformation function if I ever needed to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@georgebonnr Correct, it will not prevent a function from being called to create value, but it will prevent the PromiseState from being updated with the same value. If you want to memoize the function result, it would need to happen outside of connect() or possibly use comparison to have finer grain control of when the PromiseState is updated, but not sure it would really help much for something just based on a top-level prop. As you mentioned, the typical case inside of then or andThen will stop the function from being called unnecessarily.

Instead of bailing out of of refetchDatum() for identity requests, this changes the logic to first create a resolved promise and then handle them the same as HTTP response to support composition and other post-response handlers. This simplifies the promise chain and fixes a potential bug with network error handling previously introduced with metadata support. This is also a step in the direction to provide for generic request handlers as this moves out some of the HTTP-specifics.
@ryanbrainard
Copy link
Contributor Author

Just added a new commit that breaks out createPromise() to treat identities as a resolved Promise and work the same as HTTP fetch for post processing. This abstracts things a bit and should make it easier to introduce generic/customizable request handling.

@georgebonnr
Copy link

cool! going to go ahead and point to this branch for now.

@ryanbrainard ryanbrainard merged commit 9219ee4 into master Jan 16, 2016
@ryanbrainard
Copy link
Contributor Author

Released in v0.7.0-beta.0

@ryanbrainard ryanbrainard deleted the identity-requests branch February 2, 2016 09:04
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.

None yet

2 participants