Closed yunyu closed 4 years ago
Thanks for the report!
Clarification: by 'network request', do you mean the PATCH started by calling update({}, { myProperty: true });
? In other words, you would like the changes to be reflected before they actually commit on the server?
@ntucker Yes - that's the behavior I've seen from other optimism implementations (like Apollo).
I'd like the option to have this as well
Very interested as well
I'm always open to proposals or PRs on how to get this to work. The challenge that's got me a bit paused on solving this is what exactly to do when the request errors out. Some sort of rollback would be needed. The only solution I can think of is to have users write their own rollback algorithm which seems a bit scary as it can easily break the cache state. Perhaps if we limited the types of updates to just updating one entity record it could be done more automatically. I'd love to hear others thoughts on these challenges and what the most important use cases are for everyone.
@ntucker Frameworks like Apollo and graphql-flutter maintain a list of optimistic patches that are reapplied on every data store update. A patch is removed once its corresponding network action either succeeds or fails.
Ah, that definitely seems achievable. I'll see about getting around to this, but would accept any PRs along those lines for this to happen faster :)
No guarantees, but I'll try digging into this sometime
I thought about design a bit: could probably store like this in cacheprovider:
import { ActionTypes } from '~/types';
export default function CacheProvider({
children,
managers,
initialState,
}: ProviderProps) {
const useEnhancedReducer = createEnhancedReducerHook(
...managers.map(manager => manager.getMiddleware()),
);
const [state, dispatch] = useEnhancedReducer(masterReducer, initialState);
/** additions follow: */
// maybe this should just be part of the reducer's state so it can be handled completely from dispatches?
const [optimisticUpdates, setOptimisticUpdates] = useState([] as ActionTypes[]);
const optimisticState = useMemo(() => {
let newState = state;
for (const optimisticUpdate of optimisticUpdates) {
newState = masterReducer(newState, optimisticUpdate);
}
return newState;
}, [state, optimisticUpdates]);
/** End additions */
// if we change out the manager we need to make sure it has no hanging async
useEffect(() => {
return () => {
for (let i = 0; i < managers.length; ++i) {
managers[i].cleanup();
}
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, managers);
return (
<DispatchContext.Provider value={dispatch}>
<StateContext.Provider value={optimisticState}>{children}</StateContext.Provider>
</DispatchContext.Provider>
);
}
Other steps:
I have an idea for how to control this and would love feedback for those who are most interested in using this functionality:
Basically it would add to the fetchOptions an optimisticUpdate
member. This would be a function that takes the request params and body, and returns the expected response.
import { Resource } from 'rest-hooks';
export default class UserResource extends Resource {
static partialUpdateShape<T extends typeof Resource>(this: T) {
return {
...super.partialUpdateShape(),
options: {
...this.getFetchOptions(),
optimisticUpdate: (params, body) => body,
},
};
}
}
Thoughts? If you like give +1
@ntucker Makes sense, providing the full response is effectively what Apollo does (granted, at the hook rather than the schema level): https://www.apollographql.com/docs/react/performance/optimistic-ui/#basic-optimistic-ui
Ya by focusing around the network interface we can hopefully reduce a lot of boilerplate by keeping the definition consistent. While less code to maintain, this also means less bundle to download for users. This also has the advantage of making generated network definitions holding much more power for users.
@yunyu @kjanoudi @ChristopherMillon 4.3.0-beta.0 has optimistic updates. Docs here
Tell me how it works for you
hey @ntucker, I'm getting this:
normalizr.es.js:587 Uncaught (in promise) Error: Unexpected input given to normalize. Expected type to be "object", found "string".
at normalize (normalizr.es.js:587)
at reducer (reducer.ts:91)
at updateReducer (react-dom.development.js:16591)
at Object.useReducer (react-dom.development.js:17346)
at useReducer (react.development.js:1623)
at useEnhancedReducer (middleware.ts:18)
at CacheProvider (CacheProvider.tsx:23)
at renderWithHooks (react-dom.development.js:16260)
at updateFunctionComponent (react-dom.development.js:18347)
at beginWork$1 (react-dom.development.js:20176)
at HTMLUnknownElement.callCallback (react-dom.development.js:336)
at Object.invokeGuardedCallbackDev (react-dom.development.js:385)
at invokeGuardedCallback (react-dom.development.js:440)
at beginWork$$1 (react-dom.development.js:25780)
at performUnitOfWork (react-dom.development.js:24695)
at workLoopSync (react-dom.development.js:24671)
at performSyncWorkOnRoot (react-dom.development.js:24270)
at react-dom.development.js:12199
at unstable_runWithPriority (scheduler.development.js:697)
at runWithPriority$2 (react-dom.development.js:12149)
at flushSyncCallbackQueueImpl (react-dom.development.js:12194)
at flushSyncCallbackQueue (react-dom.development.js:12182)
at scheduleUpdateOnFiber (react-dom.development.js:23709)
at dispatchAction (react-dom.development.js:17076)
at usePromisifiedDispatch.ts:28
at SubscriptionManager.ts:120
at NetworkManager.ts:131
at dispatch (middleware.ts:38)
at NetworkManager.ts:71
My Resource
looks like this:
export default class GoalsResource extends AuthdResource {
id = undefined;
minutes = 0;
pk() {
return this.id + ' ';
}
static urlRoot = `${config.serviceUrl}/goals/`;
static getFetchKey = () => {
return { id: "daily" };
}
static updateShape() {
return {
...super.updateShape(),
schema: {},
options: {
...this.getFetchOptions(),
optimisticUpdate: (params, body) => ({
// we absolutely need the primary key here,
// but won't be sent in a partial update
id: 'daily',
...body,
}),
},
};
}
}
And I update it this way:
const updateGoal = useFetcher(GoalsResource.updateShape());
updateGoal(
{ id: 'daily' },
{ minutes: goalValue });
Any thoughts?
The Goal model looks like this, for example: {minutes: 30}
You probably want schema in updateShape to be this.asSchema()
, though that's the default so specifying it isn't even required
@ChristopherMillon does the error happen immediately or after network fetch completes?
I removed schema: {},
as you advised, and here's what Developer Tools -> Network looks like, no network error involved.
Can you get what 'action' is at that point? Either by console.log or debugging tools
Sure
So it looks like payload is what it should be (an object). Payload is the first argument to normalize.
export const normalize = (input, schema) => {
const schemaType = expectedSchemaType(schema);
if (input === null || typeof input !== schemaType) {
throw new Error(
`Unexpected input given to normalize. Expected type to be "${schemaType}", found "${
input === null ? 'null' : typeof input
}".`,
);
}
/// more
}
That's the start of normalize, which is saying typeof input is string. So I'm honestly kinda lost from what I see here.
@ChristopherMillon are you absolutely certain that particular inspected call is the one that leads to the error thrown? That line will get run multiple times and I just can't see how that value for payload could result in the error thrown.
@ChristopherMillon if you provide a sample repo with this issue I might be able to help debug it more easily
React version 16.11.0
Concurrent mode no
Describe the bug Optimism is not working on partialUpdateShape for a singleton resource.
To Reproduce
I have a singleton resource implemented as such:
When I do:
I have to wait for the network request to finish before a rerender with the updated data (myProperty is true) is triggered. This delay is very noticeable.
Expected behavior I would expect the partial update to be applied instantaneously, instead of needing to wait for the network request to finish.