graphback icon indicating copy to clipboard operation
graphback copied to clipboard

fix(packages): Adds `GraphQLNonNull` to the `modelType` for ResultList.items

Open CoreyKovalik opened this issue 3 years ago • 5 comments

This makes our DX better. As a developer, we shouldn't have to assert that MyType is not null, because it never happens.

items is never [MyType1, MyType2, null, null, MyType3] or [null, null, null, ...N]

before:

export type MyModelTypeResultList = {
  __typename?: 'MyModelTypeResultList';
  items: Array<Maybe<MyModelType>>;
  offset?: Maybe<Scalars['Int']>;
  limit?: Maybe<Scalars['Int']>;
  count?: Maybe<Scalars['Int']>;
};

after:

export type MyModelTypeResultList = {
  __typename?: 'MyModelTypeResultList';
  items: Array<MyModelType>;
  offset?: Maybe<Scalars['Int']>;
  limit?: Maybe<Scalars['Int']>;
  count?: Maybe<Scalars['Int']>;
};

Types of changes

What types of changes does your code introduce to Graphback?

  • [x] Bugfix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] Documentation Update (if none of the other choices apply)
  • [ ] Other (please specify)

Checklist

  • [x] I have read the CONTRIBUTING document.
  • [x] Lint and unit tests pass locally with my changes
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] I have added necessary documentation (if appropriate)

CoreyKovalik avatar Jun 22 '21 00:06 CoreyKovalik

ran yarn test -- -u to update snapshots

CoreyKovalik avatar Jun 22 '21 16:06 CoreyKovalik

Fully agree

wtrocki avatar Jun 22 '21 17:06 wtrocki

I tend to agree also - this would mean deviating from the GraphQL CRUD spec somewhat. Should we also propose it gets updated with this?

craicoverflow avatar Jun 22 '21 17:06 craicoverflow

I think that this was more ommision on our side. If we look into rules, hasura or aws they all asume no nulls are returned

https://graphql-rules.com/rules/output-non-null

Sad part is we need to break graphback for this change and make tons of updates in graphqlcrud. doc etc.

wtrocki avatar Jun 22 '21 17:06 wtrocki

I think that this was more ommision on our side. If we look into rules, hasura or aws they all asume no nulls are returned

https://graphql-rules.com/rules/output-non-null

Sad part is we need to break graphback for this change and make tons of updates in graphqlcrud. doc etc.

This is undoubtedly an improvement. However, you're infinitely more familiar with the codebase. Maybe this PR has opened a can of worms that is worth kicking down the road if it's going to cause a breaking change for the next release

CoreyKovalik avatar Jun 22 '21 18:06 CoreyKovalik