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

should draftprovider (experimental) be used? #3

Closed
deadcoder0904 opened this issue Dec 9, 2021 · 20 comments
Closed

should draftprovider (experimental) be used? #3

deadcoder0904 opened this issue Dec 9, 2021 · 20 comments

Comments

@deadcoder0904
Copy link
Contributor

i know it is experimental but would love to know if it should be used.

i'm using valtio to lift-state up as i need it across sibling components & would love to know if there are any plans to make it stable?

@lukesmurray
Copy link
Owner

lukesmurray commented Dec 9, 2021

You can use draftProvider as long as you use one instance of the hook as the source of truth for your synchronized state.

You may be tempted to wrap useReactQueryAutoSync in a custom hook with a draftProvider and use that hook in multiple places in your application.

// your custom hook
export const useTodosQuery = () => useReactQueryAutoSync(...)

// one place in your app
const {data} = useTodosQuery()
return <ul>{data.map(t => <li>t.text</li>)}</ul>

// another place in your app
const {data} = useTodosQuery()
<div>count {data.length}</div>

This is wrong but it will render correctly!

This would lead to duplicate save API calls, since one mutation would be issued for each instance of the hook.

The reason is, the logic for saving is local to each instantiation of the hook.

useEffect(() => {
// check that autoSave is enabled and there are local changes to save
if (autoSaveOptions?.wait !== undefined && draft !== undefined) {
saveDebounced();
}
}, [saveDebounced, draft, autoSaveOptions?.wait]);

So anytime the draft value changes, every instance of your custom hook would issue a mutate call to the server.

TLDR: The safe way to use draftProvider is to use the auto save hook in one place in the application, and then use a separate hook to access and set the draft value. You can use that separate hook safely as many times as you want, as long as you only use useReactQueryAutoSync once per draft.

// define your draft (this uses jotai)
const todosAtom = atom(undefined);

// define a custom hook accessing the draft
const useTodos = () => useAtom(todosAtom)

function App() {
  // synchronize the value in one place in your app
  const [draft_, setDraft_] = useAtom(todosAtom);
  const { draft, setDraft} = useReactQueryAutoSync(...);
}

// now you can access/set the draft wherever you want safely

// one place in your app
const [data] = useTodos()
return <ul>{data.map(t => <li>t.text</li>)}</ul>

// another place in your app
const [data] = useTodos()
<div>count {data.length}</div>

@lukesmurray
Copy link
Owner

Please let me know if that answers your question. If it does I'll add a link to this comment in the README.

@deadcoder0904
Copy link
Contributor Author

hey @lukesmurray i was trying to setup a minimal repro but took too long since there were too many moving parts but finally got it going → https://github.com/deadcoder0904/docs-autosync

i'm still not able to do it although i'm using valtio instead of jotai but rest is the same. here's how it looks:

Screenshot 2021-12-10 at 8 10 02 PM

what i want is when i click on the right-side, it shows the same docs on the center. i'm still trying to get the textarea working but yeah the DraftProvider isn't working for me. i'm probably doing something wrong.

can you take a look? while i try to manage the textarea state (which actually works in my own app but don't know why i can't type anything right now)

@lukesmurray
Copy link
Owner

@deadcoder0904 I'm sorry but I probably do not have time to debug your code. You are doing a couple of weird things. you don't pass a queryKey to your queryOptions and you are using a reference to a separate query rather than passing the query options to useReactQueryAutoSync directly. Hopefully, that is your bug but I'm not sure.

https://github.com/deadcoder0904/docs-autosync/blob/af01fbe73b6c54810b4b97114a1ef3e115817387/components/Writer.tsx#L17-L19

I don't use valtio so there may be issues there, and there are simply too many moving parts in your application for me to know if it is an issue with useReactQueryAutoSync or an issue with something else you're doing.

If you can report a reproducible issue with useReactQueryAutoSync I can try to help you.

@deadcoder0904
Copy link
Contributor Author

@lukesmurray yeah, i'll let you know when i make it working. i just quickly posted this.

i did pass useGetDocsByIdQuery({ id: state.docs.id }) inside queryFn but it gave error:

Error: Should have a queue. This is likely a bug in React. Please file an issue.

this is what i did:

const { draft, setDraft, queryResult } = useReactQueryAutoSync({
    queryOptions: {
      queryFn: async () => {
        return useGetDocsByIdQuery({ id: state.docs.id })
      },
    },

idk what to pass to queryFn when using auto-generated queries from graphql-codegen & make it work with this library.

I don't use valtio so there may be issues there

valtio is really simple. probably the easiest state management library out there haha. just mutate directly on state like state.count++ & it automatically re-renders based on something changed.

but yeah, i'll get back to making textarea work & post again 👍

@lukesmurray
Copy link
Owner

Still not sure what your issue is but you should not be passing a hook to your query function. Just use graphql codegen to generate plain fetch calls without integrating with react-query then pass those calls to useReactQueryAutoSync.

const { draft, setDraft, queryResult } = useReactQueryAutoSync({
  queryOptions: {
    queryKey: ["GetDocsById", variables],
    queryFn: fetcher<GetDocsByIdQuery, GetDocsByIdQueryVariables>(
      GetDocsByIdDocument,
      variables
    ),
  },
  mutationOptions: {
    mutationKey: "UpdateDocs",
    mutationFn: (variables?: UpdateDocsMutationVariables) =>
      fetcher<UpdateDocsMutation, UpdateDocsMutationVariables>(
        UpdateDocsDocument,
        variables
      )(),
  },
});

If I were going to take that approach I would have graphql codegen generate plain API calls (e.g. using fetch/axios). I would then pass those API calls as the queryFn or mutationFn to this hook.

@deadcoder0904
Copy link
Contributor Author

thanks for that, i managed to get quite close.

here's the error:

Invalid prisma.docs.update() invocation: An operation failed because it depends on one or more records that were required but not found. Record to update not found.

this happens because i think the mutationFn gets called directly on mount. see fetcher()() in mutationFn so id is not passed & update requires an id to mutate text.

i'm really close. the only thing that needs work is textarea that shows what i type. currently, it types out very slowly.

as regards to valtio, i'm only using 2 things from it: snap & state. basically, read from snap & mutate state.

everything is stored in store.ts which is just a class instantiated. i did match the parity of DraftProvider with valtio store so draft & setDraft match out.

updated the repo → https://github.com/deadcoder0904/docs-autosync

would be really helpful if you can take a quick look? i'll still be trying tho :)

@lukesmurray
Copy link
Owner

I didn't run your code but I don't think the mutate function is being called on init. The interesting part of this report is that your save and load take different parameters. I may extend the API to more easily support that but I have to think about the design a bit.

@deadcoder0904
Copy link
Contributor Author

deadcoder0904 commented Dec 14, 2021

The interesting part of this report is that your save and load take different parameters.

i'm confused what save & load are?

looking at the code of this library, i'm assuming save is being called when mutate is called & load is the same as draft but would be helpful if you can point it in my code so i can finally fix this bad boy!

another question, do i have to use useEffect to check if draft & setDraft changed if i don't use DraftProvider?

another thing that could be improved is the docs for DraftProvider on how it expects to be typed. i know it should be TQueryData | undefined but i don't use undefined in my store so it was throwing TS error. of course, that could be specified in the docs section.

as i had to change from:

setDocs = (docs: Docs) => {
      this.docs = docs
}

to

setDocs = (docs: Docs | undefined) => {
    if (docs) {
      this.docs = docs
    }
}

when i changed to using DraftProvider.

@deadcoder0904
Copy link
Contributor Author

deadcoder0904 commented Dec 14, 2021

I didn't run your code but I don't think the mutate function is being called on init.

it is. i just triple-checked. the error i get in chrome console on reloading the page is:

next-dev.js?3515:32 Error: 
Invalid `prisma.docs.update()` invocation:


  An operation failed because it depends on one or more records that were required but not found. Record to update not found.
    at _callee$ (Writer.tsx?a7ff:31)
    at tryCatch (runtime.js?c56e:45)
    at Generator.invoke [as _invoke] (runtime.js?c56e:274)
    at Generator.prototype.<computed> [as next] (runtime.js?c56e:97)
    at asyncGeneratorStep (Writer.tsx?a7ff:16)
    at _next (Writer.tsx?a7ff:16)
window.console.error @ next-dev.js?3515:32
overrideMethod @ react_devtools_backend.js:2540
eval @ mutation.js?f0e8:106
Promise.catch (async)
execute @ mutation.js?f0e8:102
mutate @ mutationObserver.js?5680:83
eval @ useMutation.js?623b:41
eval @ use-react-query-auto-sync.es.js?dc6e:1
s @ use-react-query-auto-sync.es.js?dc6e:1
y @ use-react-query-auto-sync.es.js?dc6e:1
b @ use-react-query-auto-sync.es.js?dc6e:1
setTimeout (async)
p @ use-react-query-auto-sync.es.js?dc6e:1
w @ use-react-query-auto-sync.es.js?dc6e:1
eval @ use-react-query-auto-sync.es.js?dc6e:1
invokePassiveEffectCreate @ react-dom.development.js?ac89:23487
callCallback @ react-dom.development.js?ac89:3945
invokeGuardedCallbackDev @ react-dom.development.js?ac89:3994
invokeGuardedCallback @ react-dom.development.js?ac89:4056
flushPassiveEffectsImpl @ react-dom.development.js?ac89:23574
unstable_runWithPriority @ scheduler.development.js?bcd2:468
runWithPriority$1 @ react-dom.development.js?ac89:11276
flushPassiveEffects @ react-dom.development.js?ac89:23447
eval @ react-dom.development.js?ac89:23324
workLoop @ scheduler.development.js?bcd2:417
flushWork @ scheduler.development.js?bcd2:390
performWorkUntilDeadline @ scheduler.development.js?bcd2:157

i have only 1 prisma.update mutation on the server (see https://github.com/deadcoder0904/docs-autosync/blob/63ca6e7a4c9d7133b4004e68276ab09c342e38b3/graphql/Docs.ts#L62-L64) & only 1 update call in Writer.tsx inside useReactQueryAutoSync hook (see https://github.com/deadcoder0904/docs-autosync/blob/63ca6e7a4c9d7133b4004e68276ab09c342e38b3/components/Writer.tsx#L47-L52):

const useWritingPad = (id: string) => {
  return useReactQueryAutoSync({
    queryOptions: {
      queryKey: ['GetDocsById', { id }],
      queryFn: fetcher<GetDocsByIdQuery, GetDocsByIdQueryVariables>(
        GetDocsByIdDocument,
        { id }
      ),
    },
    mutationOptions: {
      mutationFn: (variables?: UpdateDocsMutationVariables) =>
        fetcher<UpdateDocsMutation, UpdateDocsMutationVariables>(
          UpdateDocsDocument,
          variables
        )(),
    },
    autoSaveOptions: { wait: 1000 },
    alertIfUnsavedChanges: true,
  })
}

i log the id & text on the server & i got empty values { id: '', text: '' } which is what i set initially on loading the valtio store (see https://github.com/deadcoder0904/docs-autosync/blob/63ca6e7a4c9d7133b4004e68276ab09c342e38b3/store/index.ts#L13-L16):

class DocsStore implements IDocsStore {
  docs: Docs = {
    id: '',
    text: '',
  }
.
.
.

i also hid my other component to see if the textarea is the one causing that & it turned out to be true

apparently, getDocsById is also called which is specified in queryFn so i guess i am using it wrong but i did use it exactly as you mentioned.

i guess getDocsById should be called but not mutation so i removed the extra () & it's apparently working well. not sure if that's the right thing to do or i should return null when the id is null or '' so the prisma.update() function doesn't run.

edit: used the enabled property to not call it until id is set like enabled: id !== ''

@lukesmurray
Copy link
Owner

ok I'll take a look at this when I have time and try to find a solution.

@lukesmurray lukesmurray reopened this Dec 14, 2021
@deadcoder0904
Copy link
Contributor Author

cool, thanks luke. i just updated https://github.com/deadcoder0904/docs-autosync to use it with & without DraftProvider. see the 2 branches.

the one without DraftProvider works to update state but the one with it doesn't work. so that's probably the problem with draft & setDraft

let me know if u have any questions with my code :)

@lukesmurray
Copy link
Owner

your query function returns GetDocsByIdQuery which has the following shape.

export type GetDocsByIdQuery = {
  __typename?: "Query";
  getDocsById?:
    | { __typename?: "Docs"; id: string; text: string; published: boolean }
    | null
    | undefined;
};

but you seem to think the draft is a Doc

    const doc: Doc = {
      id: snap.doc.id,
      text: e.target.value,
    };
    state.doc = doc;
    setDraft(doc);

The underlying issue in the library is that I assume the input to your mutation is the same as the return value of your query. Typescript resolves TQueryData as the input to your mutation so you are getting the wrong types. This is a bug in my code.

TQueryData, // input to mutate is the same as the output of the query

Anyway if you change the queryFn to return the correct values your text fields update.

      queryFn: () => fetcher<GetDocsByIdQuery, GetDocsByIdQueryVariables>(
        GetDocsByIdDocument,
        { id }
      )().then(res => res.getDocsById),

@deadcoder0904
Copy link
Contributor Author

Anyway if you change the queryFn to return the correct values your text fields update.

that's it? will this fix it? or do i need to wait for the other issues to resolve?

also, was this comment regarding draftProvider?

@lukesmurray
Copy link
Owner

Yes if you change the query function to the code i provided your text fields update. You didn't have a draft provider so I didn't address any issues for the draft provider.

@deadcoder0904
Copy link
Contributor Author

deadcoder0904 commented Dec 23, 2021

You didn't have a draft provider so I didn't address any issues for the draft provider.

i do have a draft provider. i mentioned it above that i have a branch named draft-provider which wasn't working with update.

the other one was working i think but then it stopped working. will take a look now & let you know if it works fine or not :)

@deadcoder0904
Copy link
Contributor Author

but you seem to think the draft is a Doc

regarding that, i didn't think that. i just went with what the types had shown me.

@deadcoder0904
Copy link
Contributor Author

Anyway if you change the queryFn to return the correct values your text fields update.

i tried that but it gives me red-squiggly lines saying this on queryFn:

(property) QueryOptions<unknown, unknown, unknown, (string | { id: string; })[]>.queryFn?: QueryFunction<unknown, (string | {
    id: string;
})[]> | undefined
Type 'Promise<{ __typename?: "Docs" | undefined; id: string; text: string; published: boolean; } | null | undefined>' is not assignable to type 'QueryFunction<unknown, (string | { id: string; })[]>'.
  Type 'Promise<{ __typename?: "Docs" | undefined; id: string; text: string; published: boolean; } | null | undefined>' provides no match for the signature '(context: QueryFunctionContext<(string | { id: string; })[], any>): unknown'.ts(2322)
types.d.ts(35, 5): The expected type comes from property 'queryFn' which is declared here on type 'UseQueryOptions<unknown, unknown, Exact<{ id: string; text: string; }> | undefined, (string | { id: string; })[]>'

this is on my main branch, not draft-provider

@lukesmurray
Copy link
Owner

regarding that, i didn't think that. i just went with what the types had shown me.

Sorry about implying that you made a mistake. Like you said there is an issue with the types, which is now reported in #5.

i tried that but it gives me red-squiggly lines saying this on queryFn:

Here is a patch for your main branch (deadcoder0904/docs-autosync#1). The patch assumes query function always returns a Doc. In reality the query function could return null or undefined but the library assumes that mutate and query accept and return the same type. That will be fixed soon in #5.

i do have a draft provider. i mentioned it above that i have a branch named draft-provider which wasn't working with update.

Wow I missed that, I was looking for a draft provider in your main branch. I just checked out the draft provider branch and unfortunately could not figure out what was going on. I'm going to close this issue for now since I think I answered the original question "should draftprovider (experimental) be used?". Maybe the real answer is not yet. It is experimental after all. If you can create a minimal reproducible example of the draftProvider bug you are facing, please open it in a new issue and maybe we can solve it together.

I apologize for getting frustrated. I do really appreciate the bug reports, but open source is something I do for fun so please help me keep it that way by providing minimal examples. From here on out I have to set a boundary that I will not be debugging your codebase. It just is not fun for me and takes up too much time. I hope you understand.

Repository owner locked as too heated and limited conversation to collaborators Dec 23, 2021
Repository owner unlocked this conversation Dec 23, 2021
Repository owner locked and limited conversation to collaborators Dec 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants