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

createCell: Give cell results priority over cell props #4304

Merged
merged 2 commits into from
Feb 2, 2022

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Jan 30, 2022

This PR resolves #2962

It's not the solution we decided on, but it's the solution I want 😁

So, this is the setup

<JoinCell inviteCode={inviteCode} />

and a cell query like this

export const QUERY = gql`
  query JoinTeamQuery($inviteCode: String!) {
    inviteCode(code: $inviteCode) {
      code
      expiry
    }
  }
`

And finally a Success component like this

export const Success = ({ inviteCode }) => {
  return <pre>{JSON.stringify(inviteCode, null, 2)}</pre>
}

With my new implementation you'd see this:

{
  "__typename": "InviteCode",
  "code": "1NBVWN",
  "expiry": "2022-01-31T16:04:00.000Z"
}

With the old implementation all you'd get was this:

"1NBVWN"

And, finally, if you want to get at the actual props that have been over-ridden by the query result you can find them in variables:

export const Success = ({ inviteCode, variables }) => {
  return <pre>{JSON.stringify({ inviteCode, variables }, null, 2)}</pre>
}
{
  "inviteCode": {
    "__typename": "InviteCode",
    "code": "1NBVWN",
    "expiry": "2022-01-31T16:04:00.000Z"
  },
  "variables": {
    "inviteCode": "1NBVWN"
  }
}

@Tobbe Tobbe added the release:fix This PR is a fix label Jan 30, 2022
@Tobbe
Copy link
Member Author

Tobbe commented Jan 30, 2022

The alternative, that was discussed, was to throw an error if the query result name clashes with a prop name.

I think this solution is more inline with what we do if a query param clashes with a path param (path param always takes precedence). I also think it fits with RW's opinionated nature of just deciding on how things should work, that's the right fit 99 times out of 100, and provide ways to work around it for the 1%.

Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! commonProps becomes a little less useful/precise of an abstraction but I can't think of a better name right now

Copy link
Member Author

Tobbe commented Feb 1, 2022

Yeah, I know. I considered getting rid of it and just spread everything where it's used. You want me to do that instead?

Copy link
Contributor

jtoar commented Feb 1, 2022

Yeah that'd be better at this point I think

@Tobbe
Copy link
Member Author

Tobbe commented Feb 1, 2022

Yeah that'd be better at this point I think

Done!

@Tobbe Tobbe force-pushed the tobbe-cell-result-prio branch from 9e7bc81 to 1ee77f6 Compare February 1, 2022 14:39
@Tobbe
Copy link
Member Author

Tobbe commented Feb 1, 2022

Technically this is a breaking change. How vocal do we need to be about that?

@Tobbe
Copy link
Member Author

Tobbe commented Feb 1, 2022

Discussed this on a Core Team meeting today. We're good to go with this solution.

@thedavidprice
Copy link
Contributor

@Tobbe I'll leave it to you and @jtoar to handle next steps (merge, docs, etc.) Just keep me posted otherwise.

@Tobbe Tobbe force-pushed the tobbe-cell-result-prio branch from 1ee77f6 to 84865b5 Compare February 2, 2022 06:44
@Tobbe Tobbe merged commit 4625459 into redwoodjs:main Feb 2, 2022
@Tobbe Tobbe deleted the tobbe-cell-result-prio branch February 2, 2022 07:16
@jtoar jtoar added this to the next-release milestone Feb 2, 2022
dac09 added a commit to dac09/redwood that referenced this pull request Feb 2, 2022
…ize-jest-config

* 'main' of github.com:redwoodjs/redwood:
  Update dependency @types/vscode to v1.63.2 (redwoodjs#4332)
  createCell: Give cell results priority over cell props (redwoodjs#4304)
  Update dependency node-notifier to v10.0.1 (redwoodjs#4331)
  Update dependency core-js to v3.21.0 (redwoodjs#4330)
  Update dependency @clerk/clerk-js to v2.12.1 (redwoodjs#4327)
  Update dependency @clerk/types to v1.24.1 (redwoodjs#4328)
  Update dependency supertokens-node to v8.6.0 (redwoodjs#4324)
  Update prisma monorepo to v3.9.0 (redwoodjs#4329)
  Update dependency @types/node to v16.11.22 (redwoodjs#4326)
@thedavidprice
Copy link
Contributor

@Tobbe were any doc updates needed with this one? (just a nudge... no need to reply)

Copy link
Member Author

Tobbe commented Feb 2, 2022

@thedavidprice I'm already talking to Dom about docs. We'll figure it out 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
No open projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

path param overwrites cell result
3 participants