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

path param overwrites cell result #2962

Closed
Tobbe opened this issue Jul 4, 2021 · 2 comments · Fixed by #4304
Closed

path param overwrites cell result #2962

Tobbe opened this issue Jul 4, 2021 · 2 comments · Fixed by #4304

Comments

@Tobbe
Copy link
Member

Tobbe commented Jul 4, 2021

Description of bug

If one of your path params has the same name as the structure returned by a cell query, the cell result will be overwritten by the path param value.

Path:

<Route path="/join-team/{inviteCode}" page={JoinTeamPage} name="joinTeam" prerender />

SDL:

type Query {
  inviteCodes: [InviteCode!]!
  inviteCode(code: String!): InviteCode
}

Query:

export const QUERY = gql`
  query JoinTeamQuery($inviteCode: String!) {
    inviteCode(code: $inviteCode) {
      code
      team {
        id
        name
        club
      }
      coach {
        id
        user {
          firstName
          lastName
        }
      }
    }
  }
`

Success:

export const Success = ({ inviteCode }) => {
  console.log('inviteCode', inviteCode)

  // ...
}

Output

QBNIW3

Expected

{
  "__typename": "InviteCode",
  "code": "QBNIW3",
  "team": {
    "__typename": "Team",
    "id": 54,
    "name": "Team Name",
    "club": "Club Name"
  },
  "coach": {
    "__typename": "Coach",
    "id": 56,
    "user": {
      "__typename": "User",
      "firstName": "Coach FirstName",
      "lastName": "Coach LastName"
    }
  }
}

Fix

Workaround

I can easily fix this in my code by making the query look like this (renaming the result to just invite)

export const QUERY = gql`
  query JoinTeamQuery($inviteCode: String!) {
    invite: inviteCode(code: $inviteCode) {
      # ...

Proposed solution

I think RW should make the result have precedence over the path parameters. And probably also document this behavior no matter how it's decided it should be handled.

@jtoar
Copy link
Contributor

jtoar commented Dec 11, 2021

Note that this is still the case:

return <Empty {...{ ...afterQueryData, ...commonProps }} />
} else {
return <Success {...{ ...afterQueryData, ...commonProps }} />
}

Switching these two around would fix the bug. For reference, commonProps is all the stuff returned from Apollo Client's useQuery:

const commonProps = {
updating: loading,
...queryRest,
...props,
}

I think the logic for spreading commonProps over afterQueryData is that commonProps has a bunch of things that we wouldn't want users to accidentally overwrite (functions for retrying the query, the query's status, etc).

But we could take props out of commonProps, spread them first, then spread afterQueryData, and finally spread the useQuery return.

@jtoar jtoar added needs discussion and removed topic/router-&-navigation bug/confirmed We have confirmed this is a bug labels Dec 11, 2021
@jtoar jtoar added this to Triage Dec 11, 2021
@jtoar jtoar moved this to New issues in Triage Dec 11, 2021
@jtoar jtoar moved this from New issues to Needs discussion in Triage Dec 11, 2021
@jtoar jtoar moved this from Needs discussion to Todo in Triage Jan 7, 2022
@jtoar
Copy link
Contributor

jtoar commented Jan 7, 2022

We discussed this at our last meeting. The team's thoughts were that the Cell's result should win out over the path, but a better way of solving this would be to just throw some sort of a descriptive error before things get to this point. The error would be thrown if the Cell's result and the path share the same name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants