Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CoreyKovalik
Copy link
Collaborator

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?

  • 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

  • I have read the CONTRIBUTING document.
  • 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
Copy link
Collaborator Author

ran yarn test -- -u to update snapshots

@wtrocki
Copy link
Contributor

wtrocki commented Jun 22, 2021

Fully agree

@craicoverflow
Copy link

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

@wtrocki
Copy link
Contributor

wtrocki commented Jun 22, 2021

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.

@CoreyKovalik
Copy link
Collaborator Author

CoreyKovalik commented Jun 22, 2021

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants