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

[Bug?]: If first query result is null in multi-query QUERY string Cell will return empty even if other queries return results #7538

Closed
1 task
mc-spieczynski opened this issue Feb 2, 2023 · 7 comments · Fixed by #7704
Labels
bug/confirmed We have confirmed this is a bug

Comments

@mc-spieczynski
Copy link

mc-spieczynski commented Feb 2, 2023

What's not working?

When having a query as such:

export const QUERY = gql`
 
 query namedQuery($id: Int) {
  findDataInModelById($id) {
   id
  }

findManyEntities {
  id
 }
// .... etc. ...
}

If findDataInModelById returns null the Cell will return Empty component even if other subqueries return results.

If on the other hand we move such query below queries that will always return data Success component will be returned:

export const QUERY = gql`
 
findManyEntities {
  id
 }

 query namedQuery($id: Int) {
  findDataInModelById($id) {
   id
  }
// .... etc. ...
}

How do we reproduce the bug?

Create a query that accepts arguments and as FIRST subquery define one that accepts the ID (or any other unique resolver) and look for non existent ID (and returns null as result) - Cell will return Empty component. Have more than one subquery defined to see what is returned.

Then move that subquery below other subqueries that are certain to return any data. The Cell will return Success component.

What's your environment? (If it applies)

No response

Are you interested in working on this?

  • I'm interested in working on this
@mc-spieczynski mc-spieczynski added the bug/needs-info More information is needed for reproduction label Feb 2, 2023
@pvenable
Copy link
Contributor

pvenable commented Feb 2, 2023

I've found that this somewhat surprising behavior appears to be documented (at least in the code) and intentional:

https://github.com/redwoodjs/redwood/blob/v4.0.1/packages/web/src/components/createCell.tsx#L140-L158
https://github.com/redwoodjs/redwood/blob/v4.0.1/packages/web/src/components/createCell.tsx#L228-L243

It's particularly problematic with TypeScript, as in the Success path the typings indicate that the other (non-first) fields are guaranteed to be present, but in reality they are not, which can lead to runtime errors.

(One can customize the isEmpty logic for a Cell if necessary: https://redwoodjs.com/docs/cells#isempty.)

@mc-spieczynski
Copy link
Author

@pvenable I'd still consider this a bug.. I would expect all fields to be compared not only the first as it is again a kind of inconsistency based on "magic" and order of queries should not matter. In this case empty component logic is not reliable.

@jtoar jtoar added bug/confirmed We have confirmed this is a bug and removed bug/needs-info More information is needed for reproduction labels Feb 4, 2023
@jtoar
Copy link
Contributor

jtoar commented Feb 4, 2023

I hear you @mc-spieczynski. Would you prefer if we made it so that it only routes to empty if both were missing? In this case your Success component would have to handle the case where only one or the other is empty.

@mc-spieczynski
Copy link
Author

@jtoar yes. I feel that if any subquery returns result it's on us to return correct results to the user. It's either that or forcing queries to return something which always returns and then Empty will never be used.

@jtoar
Copy link
Contributor

jtoar commented Feb 6, 2023

@mc-spieczynski I'll bring it up with the team this week just to make sure it wasn't implemented that way for some odd or legacy reason, but I'd imagine we'd greenlight this change and include it in the next major

@jtoar
Copy link
Contributor

jtoar commented Feb 8, 2023

@mc-spieczynski following up, it sounds like we'll be changing the current behavior to what you expected (if there's any data, route to Success) for v5. The change to createCell won't be that hard, but we'll have to be careful about rolling it out since it's a subtle breaking change. We're planning on including some kind of check script or codemod that will scan Cells in a user's project, check if there's more than one field in a query, and warn users about the new behavior.

@mc-spieczynski
Copy link
Author

@jtoar thank you for that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants