fix: avoid naming conflict with client prop

[esteban-url]

Fixes #6946

Problem:
When you use a prop named client it will be overwritten by the graphQL client. Detailed explanation and example in the issue #6946

Solution
Renaming the prop will avoid the name conflict and allow a client prop in any cell

[Josh-Walker-GM]

Hey @esteban-url I tried this out and it definitely fixes the problem in the repo you provided in the issue 🎉

I wonder if we could solve even more potential name collisions by instead doing something like query={queryRest} instead of {...queryRest} within those various <Success />, <Loading />, <Empty />, <Failure/>? That way we would restrict query from being used but allow client and any other ones from the query results. I haven't tested this at all though so it might set everything on fire and it doesn't need to be query it could be called anything.

Any of the changes in this PR will be breaking? In theory a user could be using these additional variables within their cells in their projects and renaming these will break the user's cell in that case.

Maybe we should find a good place in the docs to add this information too? Especially if it is a breaking change.

Just my initial ideas feel free to disagree or ignore because I've got no expertise in this area. Will need the maintainers to give their feedback too of course.

[esteban-url]

@Josh-Walker-GM thanks for validating it!

queryRest is being deconstructed, each of the properties returned by useQuery(...) will them selves be passed on to each component as their own prop. We could, technically, give aliases to each one, but I think that could, potentially, create a lot more issues down the line.

I guess this could be a breaking change, I don't know how to determine if it is. All the framework tests are passing, so there's that.

Tagging @Tobbe and @jtoar as I think they are the ones that introduced and reviewed the impacted lines. Do you have any more insight or comments on this PR, and the changes Josh is proposing?

[Tobbe]

Thanks for the PR @esteban-url, and thanks for jumping in @Josh-Walker-GM!

This is definitely a breaking change. If someone was passing a prop called gqlClient to a cell it would now be overwritten. Probably not very likely, but you never know.

We knew spreading so many (and big) objects out as props could/would cause conflicts. It's the same with path- and query parameters. So far we've decided the DX win of not having to go dig for your data within objects (as you would if we wrapped everything) was worth the potential conflicts.

Having a linting rule for schema.prisma that would error out on reserved words, and also one for gql queries is another solution to this problem.

I'll bring this one up with the rest of the team.

[esteban-url]

Thanks @Tobbe! I understand the considerations the team had to make. I'll keep the PR updated until you've had time to discuss it.

[replay-io]

16 replays were recorded for c5a6f37.

0 Failed

16 Passed

requireAuth graphql checks

    ```
    locator.waitFor: Target closed
    =========================== logs ===========================
    waiting for locator('.rw-form-error-title').locator('text=You don\'t have permission to do that') to be visible
    ============================================================
    ```
  </ol>
</details>
<li><a href=https://app.replay.io/recording/c4fc4f8b-6c54-456d-8744-93d614554196>useAuth hook, auth redirects checks</a></li>
<li><a href=https://app.replay.io/recording/387c335c-6869-422e-bc68-a9b459cf6689>Check that <meta> tags are rendering the correct dynamic data</a></li>
<li><a href=https://app.replay.io/recording/55d1fba8-5f27-47d7-8166-91e83f3c219d>Check that a specific blog post is prerendered</a></li>
<li><a href=https://app.replay.io/recording/4774c031-eb87-4d06-92e7-1beb4695cb4f>Check that about is prerendered</a></li>
<li><a href=https://app.replay.io/recording/8744db95-8778-4575-885a-59a670416130>Check that homepage is prerendered</a></li>
<li><a href=https://app.replay.io/recording/0d9e5696-73c6-4b56-a7c2-0f604f615aa0>Check that you can navigate from home page to specific blog post</a></li>
<li><a href=https://app.replay.io/recording/48996c17-7084-4573-9b7a-5a3b2cf66762>Waterfall prerendering (nested cells)</a></li>
<li><a href=https://app.replay.io/recording/8670ad9a-4d0d-4136-b09c-7dbc7ba7ae63>RBAC: Admin user should be able to delete contacts</a></li>
<li><a href=https://app.replay.io/recording/1ff7e386-4cab-4554-941c-88088c657a4a>RBAC: Should not be able to delete contact as non-admin user</a></li>
<li><a href=https://app.replay.io/recording/da1ed537-d871-4778-acbc-e0f4ae20c03f>Smoke test with dev server</a></li>
<li><a href=https://app.replay.io/recording/5d55e513-4007-4063-bd97-14396ddb41ea>Smoke test with rw serve</a></li>
<li><a href=https://app.replay.io/recording/7bb0cd62-d743-4a34-a603-cdb4899663f9>Loads Cell mocks when Cell is nested in another story</a></li>
<li><a href=https://app.replay.io/recording/2e3fcd02-dc2b-4eb3-aa50-e99af5647872>Loads Cell Stories</a></li>
<li><a href=https://app.replay.io/recording/c0c67d0f-fdfc-46d2-a48d-dcff01035a60>Loads MDX Stories</a></li>
<li><a href=https://app.replay.io/recording/a268daab-e3ec-4867-b8e1-9fee93c5459e>Mocks current user, and updates UI while dev server is running</a></li>

View test run on Replay ↗︎

[jtoar]

Will prioritize getting this one discussed with the rest of the core team this week @esteban-url

[esteban-url]

thats awesome thanks @jtoar, please let me know if there is anything that could be added to this PR to help get it merged. testing/docs/codemods?

Because this isn't really "public" i think having a code mod would be the most impactful, addition, you know, in case someone uses gqlClient as a prop somewhere

[jtoar]

Hey @esteban-url, we discussed this one last week. The scope of the discussion quickly expanded from renaming just "client" to renaming all the props returned by the GraphQL client that aren't "data", "loading", or "error". But we didn't settle on a name for what that would be. We threw around "result" and "queryResult". We want to avoid putting "gql" specifically because we envision Cells being GraphQL-agnostic in a way at some point. So this one is still at a bit of a stand still, though I'm following up again this week on the discussion.

Yeah this would need docs and a codemod; it's breaking so even if it was merged as soon as this week, it wouldn't be available till v5. But you could always use it right away by upgrading to canary.

[jtoar]

@esteban-url follow up, the team wants to go with "queryResult". I know this is a bit different from what you have here in this PR. What are you thoughts about it in general? And would you like to try to take it on? No pressure though!

[esteban-url]

Not sure that adding an ad-hoc type is the best way to handle the errorCode. would love feedback on this fix 🤓

[Tobbe]

This is a change to existing behavior, right? Where exactly is errorCode coming from? Why weren't we using it before?

[esteban-url]

This showed up when running the test suite on this test: https://github.com/redwoodjs/redwood/blob/main/packages/web/src/components/createCell.test.tsx#L260

Before my change the queryRest destructuring was overriding the errorCode prop:

          <Failure
            error={error}
            errorCode={error.graphQLErrors?.[0]?.extensions?.['code'] as string}
            {...props}
            updating={loading}
            {...queryRest} // <---- Overwrites errorCode because it's one of `queryRest` members. it also overwrites error
          />

But because it's now a part of queryResult the errorCode is never set. So the test referenced above fails.

Should we be destructuring and overwriting error and errorCode?

[Tobbe]

This is a change to existing behavior, right? Where exactly is errorCode coming from? Why weren't we using it before?

[jtoar]

@esteban-url just a heads up that you're free to keep merging main into this branch but it's really not that necessary till it's close to being merged, or you see a merge conflict you want to resolve. Just trying to save you some work or vigilance

[esteban-url]

got it, thanks for the tip. Every now and then go thru my pr and update them so its no big deal, but I'll probably leave them now 😄

[Josh-Walker-GM]

Hey @esteban-url hope you don't mind me pushing to the branch.

We wanted to add a codemod for this change. This means whenever a user was using one of the parameters which now lives within queryResult it will update their cell functions to use something like {data, queryResult: {client}} in place of the previous {data, client}. @jtoar will likely review my codemod work and might also have some changes for it based on some codemod restructuring in other PRs for v5.

[esteban-url]

hey @Josh-Walker-GM no problem at all! thanks for your help getting this through!

[jtoar]

Thanks @esteban-url, and nice codemod and set of tests @Josh-Walker-GM!

[shivghai]

@esteban-url - looks like type safety is broken for this - should I create an issue for the issue?
I can see that queryResult actually works, but typescript is complaining

error TS2339: Property 'queryResult' does not exist on type 'Partial<Omit<QueryOperationResult<TraceMatrixQuery, any>, "data" | "error" | "loading"> & { updating: boolean; }> & { ...; } & Exact<...>'.

37   queryResult: { refetch },
     ~~~~~~~~~~~

[Tobbe]

@shivghai Please go ahead and create a new issue for the TS problem.
Thanks for reporting 😀