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

Still allow generic Errors in redwood forms #3946

Merged
merged 3 commits into from
Dec 20, 2021

Conversation

orta
Copy link
Contributor

@orta orta commented Dec 19, 2021

The form error property accepts any:

interface FormProps extends Omit<React.ComponentPropsWithRef<'form'>, 'onSubmit'> {
    error?: any;

But raises an exception if it doesn't conform to RWGqlError which is an internal type. The form does a validation check for the other possible property networkError (via the null check) but doesn't for graphQLErrors. This was the minimal case change, but I think a tighter change might be making:

interface RWGqlError {
  message: string
  graphQLErrors: ReadonlyArray<GraphQLError>
  networkError: Error | ServerParseError | ServerError | null
}

to

interface RWGqlError extends Error {
  graphQLErrors?: ReadonlyArray<GraphQLError>
  networkError?: Error | ServerParseError | ServerError | null
}

@Tobbe
Copy link
Member

Tobbe commented Dec 19, 2021

I'm getting the same cypress e2e error on my PR as you're getting here. I've been trying to figure out why, but no luck yet :(

@jtoar
Copy link
Contributor

jtoar commented Dec 20, 2021

@Tobbe See my comment here: #3772 (comment).

@Tobbe
Copy link
Member

Tobbe commented Dec 20, 2021

The tests all pass now with the new version of cross-undici-fetch ✅

As far as this PR goes I like it 👍
But also wouldn't mind the proposed change to make it extend Error

@dthyresson You have also looked at the gql error stuff before. What's your take?

@dthyresson
Copy link
Contributor

Thanks @orta for this PR.

FYI @dac09 and I recently spoke to the Guild about different approaches to error handling -- as the client side relies a little too much on Apollo's way of using error codes and having errors outside the data block.

Instead, they promote a way of including errors as part of the SDL and using union types so that error message and info comes back as part of the data.

See: https://blog.logrocket.com/handling-graphql-errors-like-a-champ-with-unions-and-interfaces/

But, we'll revisit this concept in V2.

Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

Thanks again @orta

@dthyresson dthyresson merged commit 99f204e into redwoodjs:main Dec 20, 2021
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Dec 20, 2021
@orta
Copy link
Contributor Author

orta commented Dec 21, 2021

Yeah, I'm an "Errors in unions" person myself - https://artsy.github.io/blog/2018/10/19/where-art-thou-my-error/

@thedavidprice thedavidprice modified the milestones: next-release, v0.41.0 Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants