Skip to content

Identity Request Promises#93

Merged
ryanbrainard merged 2 commits intomasterfrom
value-promise
May 30, 2016
Merged

Identity Request Promises#93
ryanbrainard merged 2 commits intomasterfrom
value-promise

Conversation

@ryanbrainard
Copy link
Copy Markdown
Contributor

If value is given a Promise, the PromiseState will be pending until the value or reason is settled; otherwise, the PromiseState will be resolved immediately.

This was suggested in #92 as a possible solution to @nfcampos desire to load files like fetches. It should also be compatible the expectation @neezer had in #63 that static data loads immediately. I think this is a win-win. Thoughts?

Note, I played a little bit w/ trying to unify fetch requests and identity requests and cut down on the nested conditionals, but the meta handling was getting in the way. If someone with more Promise-foo wants to take a stab at it, feel free :) Either way, I think the way it is now reads better, even though it is a bit verbose.

If `value` is given a `Promise`, the `PromiseState` will be pending until the `value` or `reason` is settled; otherwise, the `PromiseState` will be resolved immediately.
@nfcampos
Copy link
Copy Markdown
Contributor

I realised now that for this to be useful value has to be a function that returns a promise, instead of a promise, otherwise it will recompute every time anything about props changes, which seeing as these are potentially expensive operations (that's why they're async in the first place) is a deal breaker.

@ryanbrainard
Copy link
Copy Markdown
Contributor Author

@nfcampos Good point. Need to think on that.

@nfcampos
Copy link
Copy Markdown
Contributor

Assuming that any function that is assigned to value is a promise-returning function that we should call sounds too restrictive (I don't see a use case for using identity requests to inject a function, though, so maybe not?), another option is for this behaviour of promise identity requests to be under a new key promise. what do you think?

@nfcampos
Copy link
Copy Markdown
Contributor

This is definitely not something that I need in every component but I've now hit a second situation where I'd have used this, do you think this belongs in react-refetch @ryanbrainard ?

@ryanbrainard
Copy link
Copy Markdown
Contributor Author

@nfcampos is your previous comment still valid?

I realised now that for this to be useful value has to be a function that returns a promise, instead of a promise

Are you saying your would like this as is or wrapped in a function?

@nfcampos
Copy link
Copy Markdown
Contributor

value: somethingThatReturnsPromise() works inside then, andThen, etc., in functional mappings. The only place where it might be problematic is directly as a top level request, but even then it's probably fine for most uses because we now only re-run the mapPropsToRequestsToProps function if the props changed. So I think it's fine as is, what do you think?

@ryanbrainard
Copy link
Copy Markdown
Contributor Author

Yeah, I think that makes sense, is simpler, and more consistent. Pulling this in.

@ryanbrainard ryanbrainard merged commit 589c098 into master May 30, 2016
@ryanbrainard ryanbrainard deleted the value-promise branch May 30, 2016 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants