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

fix(web) type NonSuspenseCellQueryResult... #11639

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

richard-stafflink
Copy link
Contributor

@richard-stafflink richard-stafflink commented Sep 26, 2024

Description:

As of RW 7:

Issue:
When using CellSuccessProps:
queryResult lost the typings for TData it used to have in RW 6, namely queryResult.fetchMore.data had the TData type (from CellSuccessProps<TData, OperationVariables>).
This also affects queryResult.previousData

Cause:
NonSuspenseCellQueryResult no longer accepted the TData generic type, and hard-coded TData for QueryOperationResult to any.

Fix:
Pass through TData, instead of any to QueryOperationResult and ensure all other inherited types also pass down TData. Cell Props for Loading and Failure could default to any.

Before the fix:
image

After the fix:
image

@@ -52,11 +52,11 @@ export type CellProps<
>

export type CellLoadingProps<TVariables extends OperationVariables = any> = {
queryResult?: NonSuspenseCellQueryResult<TVariables> | SuspenseCellQueryResult
queryResult?: NonSuspenseCellQueryResult<any, TVariables> | SuspenseCellQueryResult
Copy link
Member

Choose a reason for hiding this comment

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

I think we probably aught to have a type parameter on CellLoadingProps for TData here as well. And ideally it should come first on CellLoadingProps, but that'd be a breaking change 😒

@@ -198,9 +198,10 @@ export type SuspendingSuccessProps = React.PropsWithChildren<
}

export type NonSuspenseCellQueryResult<
TData = any,
Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I see this PR already is breaking because of this type change here.

So what we could do is add TData as the last generic type parameter in this PR. Just to get it released in a patch or minor release.

And then we can follow up with another PR that puts the parameters in the correct order that's breaking and will be released as part of the next major release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tobbe good idea!
This is fixed now

Copy link
Contributor

Choose a reason for hiding this comment

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

@Tobbe could you confirm this is fixed per your feedback and approve to merge? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

@dthyresson I'll take a look. Thanks for the ping

Pass through `TData`, instead of `any`
@richard-stafflink
Copy link
Contributor Author

@Tobbe I fixed up the prettier error

@richard-stafflink
Copy link
Contributor Author

Anything I need to do for the failed "Check changesets" task/job?

@dthyresson dthyresson added the changesets-ok Override the changesets check label Oct 3, 2024
@dthyresson
Copy link
Contributor

Anything I need to do for the failed "Check changesets" task/job?

You could add a changeset, see https://github.com/redwoodjs/redwood/tree/main/.changesets for example content
but I added the label that skips this check.

Thanks!

@Tobbe Tobbe enabled auto-merge (squash) October 3, 2024 10:29
@Tobbe Tobbe merged commit bf55845 into redwoodjs:main Oct 3, 2024
49 checks passed
Josh-Walker-GM pushed a commit that referenced this pull request Oct 8, 2024
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changesets-ok Override the changesets check release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants