atomWithQuery depending on atomWithQuery atom doesn't work well

[jet2jet]

Description

When using atomWithQuery with an atom created with atomWithQuery (and loadable) (*), it doesn't seem to work properly.
(*) using the atom in getVariables and getPause

Reproduction

1. Clone https://github.com/jet2jet/jotai-urql-test and npm install
2. Run npm run start
3. Open http://localhost:3000 , and click Detailed button
4. Click Normal button and Detailed button for some times, and detailed list will stay loading state

Others

I wonder the following block creates an atom inside the atom getter function.
https://github.com/jotaijs/jotai-urql/blob/v0.7.1/src/common.ts#L43-L64
If I modify the code like followings, it seems to work properly. (Is it OK to use atom generators inside the atom getter function?)

Modified code

  const observableAtom = atom((get) => {
    const args = getArgs(get)
    const client = getClient(get)
    const source = getPause(get) ? null : execute(client, args)
    if (!source) {
      return null
    }
    return pipe(source, toObservable)
  })
  // Note: if the function for `initialValue` accepts `get`, these atoms could be combined into one atom
  const resultAtomWithSuspense = atomWithObservable<Result>(
    (get) => get(observableAtom),
    {}
  )
  const resultAtomWithoutSuspense = atomWithObservable<Result>(
    (get) => get(observableAtom),
    { initialValue: urqlReactCompatibleInitialState }
  )
  const baseStatusAtom = atom((get) => {
    if (!get(observableAtom)) return initialLoadAtom
    // Enables or disables suspense based off global suspense atom
    const resultAtom = get(suspenseAtom) ? resultAtomWithSuspense : resultAtomWithoutSuspense
    if (process.env.NODE_ENV !== 'production') {
      resultAtom.debugPrivate = true
    }
    return resultAtom
  })

# FYI: jotai's atomWithObservable also has calling an atom generator function inside the atom getter function:
https://github.com/pmndrs/jotai/blob/v2.4.3/src/vanilla/utils/atomWithObservable.ts#L137

[dai-shi]

I don't think it's good practice anyways? Should we just issue one GraphQL query?
But, I'm interested in the fix. Didn't look into closely yet, but it looks something.

@RIP21 what do you think?

[jet2jet]

The reproduction is a simplified pattern for confirming the bug, but I think there is a case that the query needs to be splitted due to the performance (query cost, speed (e.g. display partial data as fast as possible), server load balancing, dividing authed and non-authed query, etc.).
In this case, nesting (?) atomWithQuery would be necessary.

[scamden]

we definitely have separate queries using these. And I think we are running into this bug as well. We intermittently get an infinite spinner on our nested query. cc @fractal1729

[RIP21]

If the suggested change above works while also passes all the test cases that are there. If somebody tries a changed code against our test suite and then opens PR (and preferably adds a test case with that case) I'll gladly merge it and publish.

However, my gut feeling is that most of the waterfall queries should be sorted by adding a field resolver on the first query that will use the result of the first query to resolve the second query in one query. But, surely, it's not always possible :)

[scamden]

However, my gut feeling is that most of the waterfall queries should be sorted by adding a field resolver

i'm sure there are perf benefits to this but it's not always convenient to structure the code that way (maybe with relay it would be easier :P ). I'll do a little thinking to see if we could build that way instead though in the meantime

[scamden]

@dai-shi i for one would love a clear answer on whether this is safe as well!

is it OK to use atom generators inside the atom getter function?)

[dai-shi]

Yes, it's a valid and supported hack.

[scamden]

Hi again, I think we are experiencing another related issue here. I'm sorry I don't have a simple reproduction because it seems to be a tricky and complex race condition and only reproduces in our production environment, but it's not the first time I've seen something like this. I'll describe what we've seen in hopes that someone might have an idea on how to workaround or fix:

The basic problem is that we see an infinite render loop.
Facts:

1. We are using jotai-scope library to scope some of the atoms in the getVariables call (not sure if it is relevant)
2. Two different components in two different scopes are mounting and using the URQL atom.
3. The URQL atom is mounted both directly with useAtom and as part of other derived atoms
4. I put a breakpoint in a derived atom that reads the URQL atom and see from the stack trace that it is recomputing not because of a variables changing or a new URQL query being fired but directly from the useAtom hook in one of the components.
   a. Each time the useAtom runs it seems not to have had the value in the state cache so it goes and recomputes it, and atomWithQuery reconstructs it's baseStatusAtom that then reconstructs the atomWithObservable, which executes the urql operation. The operation is cached so it returns quickly but not before creating an unresolved promise for a split second. It seems like this causes the component to suspend briefly. Then when it resolves it rerenders and seemingly has lost the cached value for the URQL atom and so it does the whole thing again.

Could it help not to create new atoms in the read functions as described above?

If I can't get this root caused and solved I think we'll have to stop using the jotai-urql library and resort to syncing normal urql hooks to the atoms which is a huge bummer so any help here would be much appreciated.

Other things i've noticed:
Also the atomWithObservable wraps the urql result in a brand new object. The urql operation result is referentially stable but since the observable's subscribe wraps it i think that forcible recomputes at least some set of the atoms downstream (at some point the operation result is pulled off that object though so I would hope Jotai would stop recomputing at that point, but I think since many of these are promise based they get a new Promise with a new reference regardless.)

cc @bryanjclark @ehzhang