-
Notifications
You must be signed in to change notification settings - Fork 65
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
v2.0.0 #72
v2.0.0 #72
Conversation
A few more breaking changes just added:
|
README.md
Outdated
| transformed | any | Result from the transform function. Will be identical to body if transform is unprovided in the query config. | ||
| entities | object | The new, updated entities that have been affected by the query. | ||
|
||
### `requestAsync` | ||
|
||
Similarly to how mutations are triggered by dispatching `mutateAsync` actions, you can trigger requests by dispatching `requestAsync` actions with a request query config. | ||
|
||
### `redux-query/advanced` and custom network adapters | ||
You can also Promise-chain on dispatched `requestAsync` actions, but a Promise will only be returned if `redux-query` determines it will make a network request. For example, if the query config not have `force` set to `true` and a previous request with the same query key previously succeeded, then a Promise will not be returned. So be sure to always check that the returned value from a dispatched `requestAsync` is a Promise before interacting with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sentence on the second line is a bit broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks.
src/lib/update.js
Outdated
export const optimisticUpdateEntities = (optimisticUpdate, entities) => { | ||
return Object.keys(optimisticUpdate).reduce( | ||
(accum, key) => { | ||
if (optimisticUpdate[key]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes as much sense for this check to be done in updateEntities
right?
I'd also be fine with it just expecting property values to always be functions and not checking at all (this doesn't fix update: { foo: 'bar' }
or other silliness).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't new code but I don't know why this check is here. Going to try removing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
src/lib/update.js
Outdated
} else { | ||
// Default to just reverting to the initial state for that | ||
// entity (before the optimistic update) | ||
accum[key] = initialEntities[key]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this suffer from a big part of the original problem? It'll rollback entity keys that weren't updated by the mutate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. Both entity arguments are picked to scope down to just the optimistically updated keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly.
case actionTypes.REQUEST_START: { | ||
const { queryKey } = action; | ||
|
||
return omit(state, queryKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to only clear this on a successful query. I could imagine a scenario where you have some UI state or even another query config that depends on the error reducer state so you wouldn't want it cleared out straight away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my reasoning for putting it here is because it will keep it in-sync with the queries state where we reset the query state on MUTATE_START/REQUEST_START
from failed promises
Fixes superagent warnings about calling .end() multiple times.
In addition to the things mentioned in #70: