Skip to content

Fix 'PromiseState.refresh should return a new PromiseState'#150

Merged
ryanbrainard merged 1 commit intomasterfrom
fix-refreshing
Sep 23, 2016
Merged

Fix 'PromiseState.refresh should return a new PromiseState'#150
ryanbrainard merged 1 commit intomasterfrom
fix-refreshing

Conversation

@ryanbrainard
Copy link
Copy Markdown
Contributor

Fixes #149

cc: @nfcampos

Comment thread src/PromiseState.js
@@ -11,9 +11,17 @@ export default class PromiseState {
// creates as PromiseState that is refreshing
// can be called without a previous PromiseState and will be both pending and refreshing
static refresh(previous, meta) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't address it here, but was noticing how it's a little weird that meta here is only used if previous is undefined; although, it does act like resolve(value, meta) though. I believe I did that this to simplify some of the logic on the refetch size, but it makes this API a little ambiguous. Don't think it needs to be addressed now, but just calling it out for the future.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah that's a little weird

@nfcampos
Copy link
Copy Markdown
Contributor

This looks great! 👍

@ryanbrainard ryanbrainard merged commit 8bb8fc0 into master Sep 23, 2016
@ryanbrainard ryanbrainard deleted the fix-refreshing branch September 23, 2016 01:58
@ryanbrainard
Copy link
Copy Markdown
Contributor Author

@nfcampos
Copy link
Copy Markdown
Contributor

Thanks!

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